CppUnit expect exception with Assert Throw compiles with warning C4127

Question

Currently I am writing unit tests in C++ with CppUnit. Recently I needed to check that an exception is thrown in a specific case using CppUnits macro:

CPPUNIT_ASSERT_THROW(
    boost::get<FooClassInBoostVariant>(m_boostVariantFooOrBar),
    boost::bad_get);

the warning during the compilation of the test surprised me (on VS2010, but will be a warning on other compilers as well...):

warning C4127: conditional expression is constant

I looked into the macro definition of CppUnit and found the following:

do {                                                            \
  bool cpputExceptionThrown_ = false;                           \
  try {                                                         \
     expression;                                                \
  } catch ( const ExceptionType & ) {                           \
     cpputExceptionThrown_ = true;                              \
  }                                                             \
                                                                \
  if ( cpputExceptionThrown_ )                                  \
     break;                                                     \
                                                                \
  CPPUNIT_NS::Asserter::fail(                                   \
                 "Expected exception: " #ExceptionType          \
                 " not thrown.",                                \
                 CPPUNIT_SOURCELINE() );                        \
} while ( false )

Well, I totally understand how this works, the do while loop is executed only once, because of the false, and the break is used to not execute the Asserter::fail() part. But why are they doing it like this? It - of course - triggers the compiler warning, as the break condition for the while loop is obviously always "false". But isn't there a more elegant way to do this? I usually adhere to the no-warning-compilation principle, so this really bugs me.

So my question really is, why didn't they implement it like this:

{                                                               \
  bool cpputExceptionThrown_ = false;                           \
  try {                                                         \
    expression;                                                 \
  } catch ( const ExceptionType & ) {                           \
    cpputExceptionThrown_ = true;                               \
  }                                                             \
                                                                \
  if ( !cpputExceptionThrown_ ) {                               \
    CPPUNIT_NS::Asserter::fail(                                 \
                 "Expected exception: " #ExceptionType          \
                 " not thrown.",                                \
                 CPPUNIT_SOURCELINE() );                        \
  }                                                             \
}

Thanks in advance!

-Hannes

Most likely incompetence, but can also be a coding style. btw that's not the only problem with cppunit. Try to compare unsigned short integers without casting. — BЈовић, Mar 11 '13 at 07:51
I would suggest to change to a better unit testing framework (like gtest) — BЈовић, Mar 11 '13 at 07:58
which is not a helpful hint because dozens of other constraints might have led to the decision to take CppUnit in a large project years ago and thousands of unit tests are written within this framework... But starting from scratch I guess I would not choose CppUnit anymore, right. — Hannes M, Mar 11 '13 at 08:03
I don't think there is anybody who can tell you why this piece of code is written that way. Why don't you just change it? — BЈовић, Mar 11 '13 at 08:10
I did, but I wanted to know if there is a legit reason to accept this compiler warning that I am not aware of. — Hannes M, Mar 11 '13 at 08:13

Arne Mertz · Answer 1 · 2013-03-11T09:24:23.507

The reason is to make the assertion one statement. Consider these two uses of the macro:

CPPUNIT_ASSERT_THROW(foo(), MyException);  // a
CPPUNIT_ASSERT_THROW(foo(), MyException)   // b - without trailing `;`!
doSomething();

With their code, you'd get an error with //b, since the code expands to do { ... } while (false) doSomething(); - you'd be missing the ; after the condition.

With your code, //b would happily compile, but //a could give you an "empty statement" warning, since that line would expand to { ... };, with the superfluos ; after the block.

Why they force you to use //a I don't know - but I like //b way more because it's just consistent to have a ; after each line. One does not have to distinguish lines with assertions from normal statements.

PS: I am not sure but there might be more differences between { ... } blocks and do {...} while(false) statements that will allow to put an assertion macro in places where simple blocks are not allowed.

Edit: with C++11, you could use a lambda (define and call it in one place):

#define CPPUNIT_ASSERT_THROW(expression, ExceptionType)         \
[&]() -> void {                                                 \
  bool cpputExceptionThrown_ = false;                           \
  try {                                                         \
     expression;                                                \
  } catch ( const ExceptionType & ) {                           \
     cpputExceptionThrown_ = true;                              \
  }                                                             \
                                                                \
  if ( cpputExceptionThrown_ )                                  \
     return;                                                    \
                                                                \
  CPPUNIT_NS::Asserter::fail(                                   \
                 "Expected exception: " #ExceptionType          \
                 " not thrown.",                                \
                 CPPUNIT_SOURCELINE() );                        \
}()

However, there might be caveats, e.g. due to the lambda capturing the variables you use in the expression.

Lambdas are a cool idea, but unfortunately I can't use C++11 in this project :-/ — Hannes M, Mar 12 '13 at 05:54

score 1 · Accepted Answer · answered Mar 11 '13 at 08:33

OK, I guess I found the answer by myself:

http://cnicholson.net/2009/02/stupid-c-tricks-adventures-in-assert/ gives the explanation.

It is actually a common practice to wrap multi-line macros in do { } while (false);. This is a workaround to allow for using those macros, e.g., in un-braced if else.

if (condition_a)
    MULTI_LINE_MACRO();
else
    MULTI_LINE_MACRO_2();

The result would be that unexpectedly only the first line gets executed, which definitely leads to unexpected behaviour. So they were not entirely incompetent I guess...

http://kernelnewbies.org/FAQ/DoWhile0 also explains why my solution would not work. The MULTI_LINE_MACRO(); within the if would expand, e.g., to if (condition_a) { /* macro stuff */ } ; else // << never executed because of the ; above.

So I guess I have to disable the warning. GCC has a workaroud for this ( ({ MACRO }) ), called a Statement Expression, but I don't think this works on VS2010.

see my edit to my answer below - you can use lambdas if you have C++11 support — Arne Mertz, Mar 11 '13 at 09:25

CppUnit expect exception with Assert Throw compiles with warning C4127

2 Answers2