RETURN_IF_FAILED macro

[Xiaohan Wang (王消寒)]

This has been previously discussed in chromium-dev@ and partially in a separate thread on logging.

I am reviewing some Windows code and saw macros wrapping Windows API calls that's similar to RETURN_IF_FAILED in Windows Implementation Libraries (WIL).

RETURN_IF_FAILED(Foo1());

RETURN_IF_FAILED(Foo2());

RETURN_IF_FAILED(Foo3());

vs.

HRESULT hr = Foo1();

if (FAILED(hr))

  return hr;

hr = Foo2();

if (FAILED(hr))

  return hr;

hr = Foo3();

if (FAILED(hr))

  return hr;

Usually I am not a fan of having "return" in macros, which harms readability IMHO, though I can't find the actual style guide against it. The current style guide only says "try not to use macros that expand to unbalanced C++ constructs", which isn't exactly the case here.

The real issue here is that without this macro, the code would be much longer with a lot of if/returns. (Some functions I am looking at have ~20 Windows API calls.) This seems to hurt readability in a different way. Here's an existing example. It's even worse since it does logging, which is a separate topic.

FWIW, there's already similar code in Chromium, e.g. RCHECK and RETURN_ON_FAILURE in media/. But overall the usage is pretty rare. 

In Google internal code, there's also RETURN_IF_ERROR which seems to be used a lot.

Shall we ban this macro, or recommend using it for Win API calls, or treat it case-by-case, e.g. only use it in Win-API-heavy functions/files? Shall we update the style guide to make this more clear?

Best,

Xiaohan

[Samuel Huang]

If the functions have identical return types, how about having an array / vector of function pointers (using lambda to adapt if needed) (or Closures), then loop over them in one place?

  HRESULT (*tasks[])() = {

    Foo1,

    []() -> HRESULT { return Foo2("bar"); },

    Foo3,

  };

  for (auto task : tasks) {

    HRESULT hr = task();

    if (FAILED(hr))

      return hr;

  }

This is also an opportunity to make the code asynchronous, if the function calls are blocking and take a long time.

--

Samuel Huang

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YO-7-Z_71kJVDPgZ9DCgiPZeDs7jnpKo3iBid5%3Doyo3VA%40mail.gmail.com.

[Peter Kasting]

On Wed, Jan 8, 2020 at 6:52 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

HRESULT hr = Foo1();

if (FAILED(hr))

  return hr;

hr = Foo2();

if (FAILED(hr))

  return hr;

hr = Foo3();

if (FAILED(hr))

  return hr;

Usually I am not a fan of having "return" in macros, which harms readability IMHO, though I can't find the actual style guide against it. The current style guide only says "try not to use macros that expand to unbalanced C++ constructs", which isn't exactly the case here.

The real issue here is that without this macro, the code would be much longer with a lot of if/returns. (Some functions I am looking at have ~20 Windows API calls.) This seems to hurt readability in a different way. Here's an existing example. It's even worse since it does logging, which is a separate topic.

Are you proposing a family of macros?  I ask because it looks like your linked example isn't actually returning an HRESULT, so a macro like RETURN_IF_FAILED that returns one isn't going to work.

For cases like this, it seems like the following code doesn't require defining new macros and is still brief:

if (SUCCEEDED(Foo1()) &&

    SUCCEEDED(Foo2()) &&

    SUCCEEDED(Foo3()) && ... {

  DoSuccessThing();

}

As necessary for debugging, it's pretty easy to change SUCCEEDED here to a macro that logs the supplied value on failure.  Whether that's permissible on checkin or should be done locally when debugging is more what the logging thread is about.

The case where we do want to return an HRESULT is more challenging to find alternate solutions for.  Samuel's proposal is one route, although it makes chaining the effects of one function call into subsequent ones tricky.  Another route is to use the (legal but rarely-used) single-line conditional format to decrease vertical space:

HRESULT hr = Foo1();

if (FAILED(hr)) return hr;

hr = Foo2();

if (FAILED(hr)) return hr;

hr = Foo3();

if (FAILED(hr)) return hr;

PK

[Xiaohan Wang (王消寒)]

Sorry I wasn't clear. I am using these two simple examples to show the core problem. In reality, you can have various branching (e.g. if/else blocks) where both suggestions won't work. Here's a more realistic example (the CHK is similar to RETURN_IF_FAILED in this discussion).

[Peter Kasting]

On Thu, Jan 9, 2020 at 8:48 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Sorry I wasn't clear. I am using these two simple examples to show the core problem. In reality, you can have various branching (e.g. if/else blocks) where both suggestions won't work. Here's a more realistic example (the CHK is similar to RETURN_IF_FAILED in this discussion).

It's a challenging problem in general, and I don't know that any one answer is best everywhere.  It seems like you're considering the right factors, and as you note the style guide allows for different routes here, so whatever you decide should be OK.

One tactic that might be helpful in reducing the need for macros is to try to avoid propagating system types or information out of functions unless necessary.  So where things return HRESULT, do callers need that level of detail, or can they just get a bool "operation failed" or Optional or something?  If the returned value is primarily used for logging, is it already logged in the child function?  This might open more avenues for rewriting the code in a way that either doesn't use macros or can use shorter/simpler ones.

But in the limit, you may wind up with a macro really being the best possible answer in some scenario.  If so go for it.

PK

[Samuel Huang]

How about making use of short-circuiting, along with a helper class to track states? Then you can have just one return at the end. Example:

================================

#include <cstdio> using HRESULT = int; constexpr HRESULT S_OK = 0; constexpr HRESULT S_FAIL = 1; class WinOkay { public: WinOkay() {} ~WinOkay() {} operator bool() { return last_result_ == S_OK; } bool operator()(HRESULT res, const char* err_msg) { if (res != S_OK) { last_result_ = res; err_msg_ = err_msg; return false; } return true; } HRESULT verdict() { if (last_result_ != S_OK && *err_msg_) { printf("Error message: %s\n", err_msg_); } return last_result_; } private: HRESULT last_result_ = S_OK; const char* err_msg_ = ""; }; HRESULT fun1() { printf("Calling fun1()\n"); return S_OK; } HRESULT fun2() { printf("Calling fun2()\n"); return S_OK; } HRESULT fun3() { printf("Calling fun3()\n"); return S_FAIL; } HRESULT fun4() { printf("Calling fun4()\n"); return S_OK; } HRESULT fun5() { printf("Calling fun5()\n"); return S_OK; } HRESULT fun() { WinOkay ok; ok && ok(fun1(), "Failed in fun1()."); if (ok && ok(fun2(), "Failed in fun2().")) { ok && ok(fun3(), "Failed in fun3()."); } ok && ok(fun4(), "Failed in fun4()."); ok && ok(fun5(), "Failed in fun5()."); return ok.verdict(); } int main() { return fun(); }

================================

--

Samuel Huang

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAq%3Da8jUtKCRJonA9-ptWzqasL2G%2BbAQ%2BZh30TF0WyY%2BQ%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

On Fri, Jan 10, 2020 at 8:17 AM Samuel Huang <hua...@chromium.org> wrote:

How about making use of short-circuiting, along with a helper class to track states? Then you can have just one return at the end. Example:

================================

#include <cstdio> using HRESULT = int; constexpr HRESULT S_OK = 0; constexpr HRESULT S_FAIL = 1; class WinOkay { public: WinOkay() {} ~WinOkay() {} operator bool() { return last_result_ == S_OK; } bool operator()(HRESULT res, const char* err_msg) { if (res != S_OK) { last_result_ = res; err_msg_ = err_msg; return false; } return true; } HRESULT verdict() { if (last_result_ != S_OK && *err_msg_) { printf("Error message: %s\n", err_msg_); } return last_result_; } private: HRESULT last_result_ = S_OK; const char* err_msg_ = ""; }; HRESULT fun1() { printf("Calling fun1()\n"); return S_OK; } HRESULT fun2() { printf("Calling fun2()\n"); return S_OK; } HRESULT fun3() { printf("Calling fun3()\n"); return S_FAIL; } HRESULT fun4() { printf("Calling fun4()\n"); return S_OK; } HRESULT fun5() { printf("Calling fun5()\n"); return S_OK; } HRESULT fun() { WinOkay ok; ok && ok(fun1(), "Failed in fun1()."); if (ok && ok(fun2(), "Failed in fun2().")) { ok && ok(fun3(), "Failed in fun3()."); } ok && ok(fun4(), "Failed in fun4()."); ok && ok(fun5(), "Failed in fun5()."); return ok.verdict(); } int main() { return fun(); }

================================

Thanks for the suggestions. I guess the requirement that you have to check |ok| everywhere afterwards is troublesome. Also, this prevents us from returning early, which we favor a lot. Actually, anywhere in your function, you can do

if (!ok)

  return ok.verdict();

But then that's back to the case where we don't use the macro.

[Samuel Huang]

I suppose that we have the following conflicting objectives:

• Short code.
• Being explicit about return.
• Expressive flexibility (to pass params, or conditional calls).

The "WinOkay" method satisfies the above, but is weak in:

• Avoid incessant checks

Anyway, it looks like you're settling with the original method (which I have no objection to).

More thinking (for fun): In JavaScript one would likely use Promises. If generators are available then one can yield the return value for each call, and a caller can pump the loop and handle the error. Ah language features envy...

[dan...@chromium.org]

On Fri, Jan 10, 2020 at 2:35 PM Samuel Huang <hua...@chromium.org> wrote:

I suppose that we have the following conflicting objectives:

• Short code.

I would always prefer obvious code over short code.

[Jeremy Roman]

On Fri, Jan 10, 2020 at 2:40 PM <dan...@chromium.org> wrote:

On Fri, Jan 10, 2020 at 2:35 PM Samuel Huang <hua...@chromium.org> wrote:

I suppose that we have the following conflicting objectives:

• Short code.

I would always prefer obvious code over short code.

+1. Short is nice insofar as it lets you fit more of the code in your head at once, but clarity is the goal. I'm fairly unconvinced that just having a (D)CHECK that something didn't fail (if we believe it cannot) or a conditional statement (if we believe it might) isn't the right solution. That's what we do nearly everywhere else.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQVOjnpwJJX8JnPQMz7T%2BBgS-4QLh_Wpo57a_fsLYGbRQ%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

I hope I didn't leave the impression that I was purely looking for shorter code. I was more concerned about readability than the length of code. I felt that having too many "if(FAILED(...)) return" breaks the flow of the code and makes things harder to read and follow.

I don't think we can simply DCHECK Windows APIs even for simple things like IMFAttributes::SetUINT32(). A CHECK() could cause the GPU/Browser process to crash, which also seems to violate the current recommendation.

I agree with pkasting@ that probably we should have some ways to limit the need for such a macro. For example, having a helper class to wrap IMFAttributes to make setting values easier.

With that, I'll allow RETURN_IF_FAILED for now for only some new Win-API-heavy files in media/. Then I'll spend some time to see whether I can reduce the use of it or even eliminate it altogether.

[dan...@chromium.org]

On Fri, Jan 10, 2020 at 5:12 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I hope I didn't leave the impression that I was purely looking for shorter code. I was more concerned about readability than the length of code. I felt that having too many "if(FAILED(...)) return" breaks the flow of the code and makes things harder to read and follow.

I don't think we can simply DCHECK Windows APIs even for simple things like IMFAttributes::SetUINT32(). A CHECK() could cause the GPU/Browser process to crash, which also seems to violate the current recommendation.

Why can't we? Many functions can fail depending on their inputs, but if we know and control their inputs then we can expect them to always succeed. This one in particular can't fail so why can't we expect it to always succeed? Is this for future compatibility or excessive defensiveness or something else?

[Xiaohan Wang (王消寒)]

On Fri, Jan 10, 2020 at 2:17 PM <dan...@chromium.org> wrote:

On Fri, Jan 10, 2020 at 5:12 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I hope I didn't leave the impression that I was purely looking for shorter code. I was more concerned about readability than the length of code. I felt that having too many "if(FAILED(...)) return" breaks the flow of the code and makes things harder to read and follow.

I don't think we can simply DCHECK Windows APIs even for simple things like IMFAttributes::SetUINT32(). A CHECK() could cause the GPU/Browser process to crash, which also seems to violate the current recommendation.

Why can't we? Many functions can fail depending on their inputs, but if we know and control their inputs then we can expect them to always succeed. This one in particular can't fail so why can't we expect it to always succeed? Is this for future compatibility or excessive defensiveness or something else?

I actually explicitly asked MS engineer on this one. Some of these calls might allocate resources which could fail. Even if we get some guarantee from MS on this one, the bigger question is whether we should get into the business of expecting whether an Win API can fail or not. How scalable will that be?

[K. Moon]

We do a similar thing for memory allocation, though: In theory, memory allocation failure is recoverable, but in practice, we just crash. Are there any parallels here?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YP0tGbAp3sowiwKRqCxrEhwM0x0d-r15cme7F8Q%3DWP_nA%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

You are right. But resource allocation is just an example here. The bigger question is why we should get into the business checking whether each Win API can or cannot fail? Aren't we supposed to code against the API, not the implementation?

[Samuel Huang]

Re. "short code" vs. "obvious code": We're talking about two different things here: You're absolutely right that stuff like "a ^= b ^= a ^= b" should be avoided. But the issue at hand is trying to follow the "don't repeat yourself" principle. We're seeking an abstraction to solve the problem of repetition. If there's a suitable existing solution then we can use it. If not, then we evaluate the cost / benefit of various solutions (if brainstorming) or the proposed one (if announcing / bargaining). The solution may be useful elsewhere (other WinAPI usage or beyond), potentially leading to wider adoption down the road.

On Fri, Jan 10, 2020 at 4:17 PM Jeremy Roman <jbr...@chromium.org> wrote:

On Fri, Jan 10, 2020 at 2:40 PM <dan...@chromium.org> wrote:

On Fri, Jan 10, 2020 at 2:35 PM Samuel Huang <hua...@chromium.org> wrote:

I suppose that we have the following conflicting objectives:

• Short code.

I would always prefer obvious code over short code.

+1. Short is nice insofar as it lets you fit more of the code in your head at once, but clarity is the goal. I'm fairly unconvinced that just having a (D)CHECK that something didn't fail (if we believe it cannot) or a conditional statement (if we believe it might) isn't the right solution. That's what we do nearly everywhere else.

• Being explicit about return.
• Expressive flexibility (to pass params, or conditional calls).

The "WinOkay" method satisfies the above, but is weak in:

• Avoid incessant checks

Anyway, it looks like you're settling with the original method (which I have no objection to).

More thinking (for fun): In JavaScript one would likely use Promises. If generators are available then one can yield the return value for each call, and a caller can pump the loop and handle the error. Ah language features envy...

On Fri, Jan 10, 2020 at 12:30 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

On Fri, Jan 10, 2020 at 8:17 AM Samuel Huang <hua...@chromium.org> wrote:

How about making use of short-circuiting, along with a helper class to track states? Then you can have just one return at the end. Example:

================================

(...) ================================

Thanks for the suggestions. I guess the requirement that you have to check |ok| everywhere afterwards is troublesome. Also, this prevents us from returning early, which we favor a lot. Actually, anywhere in your function, you can do

if (!ok)

  return ok.verdict();

But then that's back to the case where we don't use the macro.

 

--

Samuel Huang

On Thu, Jan 9, 2020 at 3:15 PM Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Jan 9, 2020 at 8:48 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Sorry I wasn't clear. I am using these two simple examples to show the core problem. In reality, you can have various branching (e.g. if/else blocks) where both suggestions won't work. Here's a more realistic example (the CHK is similar to RETURN_IF_FAILED in this discussion).

It's a challenging problem in general, and I don't know that any one answer is best everywhere.  It seems like you're considering the right factors, and as you note the style guide allows for different routes here, so whatever you decide should be OK.

One tactic that might be helpful in reducing the need for macros is to try to avoid propagating system types or information out of functions unless necessary.  So where things return HRESULT, do callers need that level of detail, or can they just get a bool "operation failed" or Optional or something?  If the returned value is primarily used for logging, is it already logged in the child function?  This might open more avenues for rewriting the code in a way that either doesn't use macros or can use shorter/simpler ones.

But in the limit, you may wind up with a macro really being the best possible answer in some scenario.  If so go for it.

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAq%3Da8jUtKCRJonA9-ptWzqasL2G%2BbAQ%2BZh30TF0WyY%2BQ%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

To close the loop. We've landed some code in src/media/ to use RETURN_IF_FAILED when we deal with a lot of Windows API. Here's an example.

[Daniel Cheng]

While I can understand why code might want to use some of those macros, the side effects seem quite surprising to me.

RETURN_ON_FAILURE checks a boolean.

RETURN_ON_HR_FAILURE checks SUCCEEDED(hresult).

RETURN_IF_FAILED checks SUCCEEDED(hresult).

The first two call mf::LogDXVAError(), while the latter does not. In addition, while all the macros already DLOG(ERROR), mf::LogDXVAError() does an additional LOG(ERROR) with a hardcoded but misleading file name.

In short, while the macros certainly save typing, they seem to be quite easy to accidentally use incorrectly, especially the FAILURE vs FAILED distinction.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YNyh3o9CNQ%2BEPb-ASMVsURApaE9WVjpCb4QkTVwXQaNkw%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

This thread is about the new RETURN_IF_FAILED and when/how to use it.

Other macros like RETURN_ON_FAILURE and RETURN_ON_HR_FAILURE predate me and I totally agree there are a lot of issues with them. Especially mf::LogDXVAError() actually reports a DXVA related UMA! And I already see some abuse of these macros. I've found this yesterday and landed a CL to fix it. There's more cleanup to do and I plan to do them incrementally.