Skip to content

Conversation

@oldnewthing
Copy link
Member

@oldnewthing oldnewthing commented Jun 30, 2021

Like check_pointer, check_hresult accepts multiple successful values. The check_pointer function returns the successful pointer, but check_hresult swallows the successful result, which makes using check_hresult inconvenient to use with COM methods that return S_FALSE. For example:

if (hresult error = root->SelectSingleNode(query, node.put()); check_hresult(error), error == S_OK)
{
    process(node);
}

Extending check_hresult to return the successful hresult simplifies the code:

if (check_hresult(root->SelectSingleNode(query, node.put())) == S_OK)
{
    process(node);
}

I think it is unlikely that anybody has been doing decltype(check_hresult()), so changing the return value should be non-breaking.

Like `check_pointer`, `check_hresult` accepts multiple successful
values. The `check_pointer` function returns the successful pointer,
but `check_hresult` had swallowed it, which made using `check_hresult`
inconvenient when interoperating with things like COM enumerators that
use `S_FALSE`. For example:

```cpp
com_ptr<IShellItem> item[1];
hresult error;
while (check_hresult(error = e->Next(1, item[0].put(), nullptr)), error == S_OK)
{
    do_stuff(item);
    item[0] = nullptr; // prepare for next iteration
}
```

Letting `check_hresult` return the `hresult` simplifies the code:

```cpp
com_ptr<IShellItem> item[1];
while (check_hresult(e->Next(1, item[0].put(), nullptr)) == S_OK)
{
    do_stuff(item);
    item[0] = nullptr; // prepare for next iteration
}
```
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kennykerr kennykerr merged commit 837ac89 into master Jun 30, 2021
@kennykerr kennykerr deleted the check-hresult branch June 30, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants