Missing brackets round expression in macro

[billyquith]

In the REQUIRE macro internals there are missing brackets around the expr (line 27) when it is evaluated. If you compared 2 booleans then the <= operator precedence is higher than == and it fails.

include/internal/catch_capture.hpp

34          Catch::ResultBuilder __catchResult( macroName, CATCH_INTERNAL_LINEINFO, #expr, resultDisposition ); \
35          try { \
36              CATCH_INTERNAL_SUPPRESS_PARENTHESES_WARNINGS \
37              ( __catchResult <= expr ).endExpression(); \

Solution, add brackets.

34          Catch::ResultBuilder __catchResult( macroName, CATCH_INTERNAL_LINEINFO, #expr, resultDisposition ); \
35          try { \
36              CATCH_INTERNAL_SUPPRESS_PARENTHESES_WARNINGS \
37              ( __catchResult <= (expr) ).endExpression(); \

[martinmoene]

Writing the expression in parenthesis defeats CATCH' expression decomposition.

[billyquith]

Writing the expression in parenthesis defeats CATCH' expression decomposition.

What am I supposed to be reading on that page that makes this issue different?

[edit] My use case is:

    SECTION("metadata can be compared")
    {
        const ponder::Class& class1 = ponder::classByType<MyClass>();
        const ponder::Class& class2 = ponder::classByType<MyClass2>();

        REQUIRE(class1 == class1);
        REQUIRE(class1 != class2);
        REQUIRE(class2 != class1);
    }    

Catch was trying to convert the rhs of the first REQUIRE test to a string to compare with the internal <= test. This requires that you know how the guts of Catch works. Outwardly, I just want to make the comparison, like the example in the link above. e.g.

CHECK( str == "string value" );
CHECK( thisReturnsTrue() );
REQUIRE( i == 42 );

[martinmoene]

Writing the expression in parenthesis defeats CATCH' expression decomposition.

What am I supposed to be reading on that page that makes this issue different?

Well, that what's on that page doesn't work anymore as described:

Without parenthesis __catchResult <= expr:

prompt>g++ -Wall -o main.exe main.cpp && main.exe --success --reporter=compact
main.cpp:9: passed: Factorial(1) == 1 for: 1 == 1
main.cpp:10: passed: Factorial(2) == 2 for: 2 == 2
main.cpp:11: passed: Factorial(3) == 6 for: 6 == 6
main.cpp:12: passed: Factorial(10) == 3628800 for: 3628800 (0x375f00) == 3628800 (0x375f00)
Passed 1 test case with 4 assertions.

With parenthesis __catchResult <= (expr):

prompt>g++ -Wall -o main.exe main.cpp && main.exe --success --reporter=compact
main.cpp:9: passed: Factorial(1) == 1 for: true
main.cpp:10: passed: Factorial(2) == 2 for: true
main.cpp:11: passed: Factorial(3) == 6 for: true
main.cpp:12: passed: Factorial(10) == 3628800 for: true
Passed 1 test case with 4 assertions.

[martinmoene]

Usually the 'problem' you decribe is solved via parenthesis locally in the test:

REQUIRE( (class1 == class1) );

[billyquith]

Do you think that is a bug or not? Please explain.

[martinmoene]

__catchResult <= expr without the parenthesis is fundamental to CATCH' capability to present Factorial(3) == 6 as 6 == 6.

To get an impression of how the expression decomposition works, you may have a look at lest_decompose and follow the use of expression_decomposer.

Kevlin Henney presented the idea in Rethinking Unit Testing in C++ (Video).

[billyquith]

If you'd written this description to start with, or it was included in the docs we probably wouldn't have this confusion. Ok, now I understand why it was trying to coerce my expression into a string, which caused my problem.

In which case I still think there is a problem, that the documentation needs updating.

It seems that docs make a big selling point of there being less comparison macros. But, using other systems, if I'd written REQUIRE( class1 == class1 ); I wouldn't expect to see this problem, rather, if it was REQUIRE_EQUAL( class1, class1); style. The purpose of this decomposition needs explaining and this gotcha needs mentioning.

[martinmoene]

(It's not always easy to assess how much information is required to clear-up things for someone.)

It's not the first time this confusion surfaces. Perhaps you can suggest such an improvement to the documentation via a PR?

cheers,
Martin

[philsquared]

I'm not quite sure what the original issue was.
You said, "If you compared 2 booleans then the <= operator precedence is higher than == and it fails."
Fails in what way? Comparing two bools works fine for me so I'm surprised if that's the actual issue.

You later give an example of comparing two objects of your own classes, and mention string conversions. Is this where the actual issue arises? You don't say whether the string conversion is working or not, or causing a problem or not. The string conversion is not actually part of the comparison. The comparison is on the actual typed data. The strings are used for reporting on what was actually tested (and should fall back to "{?}" if it can't automatically deduce how to convert it - there are ways to teach it how if that's something you'd like to do - we can pick that up separately. The point is it shouldn't stop the code from working).

In general Catch does "just work" without you having to know how it works internally. It's true that it's doing a fair bit internally to achieve that - in ways that are more complex than you might expect - which is unfortunate on those rare occasions that the abstraction does leak.
I usually do recommend added extra brackets in those cases - to just get things working without having to dig deeper. But that's a workaround and, as Martin says, you lose richness of output when you do that.

So I'm keen to get to the bottom of what's actually going wrong for you. Maybe it will inform tweaks to the docs. Maybe we can fix something in the implementation.

[billyquith]

I think the problem may be related to opaque objects that do not coerce into strings. I think I was comparing two objects and one didn't have a string representation. Putting brackets around these tests makes them work.

I'll try and investigate further when I have some time. Cheers.

[billyquith]

My issue is this: https://github.com/philsquared/Catch/blob/master/docs/tostring.md

Initially it wasn't obvious because I have a lot of templates. I have missing template specialisations for string conversion for user objects. In these cases I have to use double brackets.

I'll wrap Catch in a header and add some test-only string conversion code.

Thanks for following up.

[billyquith]

Hmm. I'll just keep this open for a response.

Don't suppose there is any way of catching things that don't coerce into strings at compile time? My tests compile ok but fail because of lack of string conversion.

[lightmare]

There are no parentheses around expr by design, they're not missing.

CHECK( a == b );
// expands to:
( __catchResult <= a == b ).endExpression();
// which, because comparison operators have equal precedence and are left-associative,
// is equivalent to:
( (__catchResult <= a) == b ).endExpression();

And this is how Catch is able to capture (using overloaded operators) and report the values of a and b.

FAILED:
  CHECK( a == b )
with expansion:
  1 == 2

If, on the other hand, there were parentheses around expr, then

CHECK( a == b );
// would expand to
( __catchResult <= (a == b) ).endExpression();

and Catch would never see the individual values of a and b. All it would see would be the result of the expression (a == b), that is true or false.

FAILED:
  CHECK( (a == b) )
with expansion:
  false

[lightmare]

My tests compile ok but fail because of lack of string conversion.

That's weird. Can you give an example, please?

[billyquith]

Ok, it seems this is because I have a competing string coercion system. I was getting modified behaviour because it wasn't being included in all the tests. Adding a Catch wrapper with string conversion extensions changed the behaviour. I think I'll just have to avoid this expr design for the cases where this is a problem.