Spot the bug - control flow macro

Spot the bug - control flow macro

  • Comments 4

Found this in a code review.

Well, in all candor, I didn't find it; our automated code review tool found it.  But I'm taking credit for it because I read the output of the automated code review tool. :-)

Code rewritten because I'm too lazy to look it up.

// macros.h
#define ENSURE_EQUAL(x, y) if (x != y) { bail(); }

// implementation.cpp
...
hr = foo();
ENSURE_EQUAL( foo_supported() ? S_OK : E_FOO_NOT_SUPPORTED, hr );
...

Leave a Comment
  • Please add 8 and 5 and type the answer here:
  • Post
  • Ugh.

    if (foo_supported() ? S_OK : (E_FOO_NOT_SUPPORTED != hr)) ...

  • As always, missing parentheses.  I think this should become a preprocessor warning.

  • How did the code review tool catch it?  What criterion did it invoke?

  • PREfast (msdn.microsoft.com/.../ms933794.aspx) caught this error, sort of by mistake.

    The type of the (a ? b : c) expression is the common type of b and c; if b and c are of different types then an attempt is made to implicitly convert c to the same type as b.

    PREfast saw the (foo_supported() ? S_OK : (E_FOO_NOT_SUPPORTED != hr)) expression and said "hmm, b is an HRESULT and c is a boolean value.  Yes, there's an implicit conversion from boolean to HRESULT but it merits a warning."

    I saw the "boolean being implicitly converted to HRESULT" warning on the ENSURE_EQUAL(...) line, was confused, and dug further until I realized what was going on.

Page 1 of 1 (4 items)