ResultBuilder::reconstructExpression() checks for impossible value: "!"

[RossBencina]

The following comparison is present in ResultBuilder::reconstructExpression():

catch_result_builder.hpp: 147

   else if( m_exprComponents.op != "!" ) {

However, as far as I can tell, m_exprComponents.op can never be equal to "!".

m_exprComponents.op is constructed as an empty string, and after that is only set via calls to ResultBuilder::setOp. There are two calls to setOp:

catch_capture.hpp(139):                .setOp( "matches" ) \
catch_expression_lhs.hpp(95):      .setOp( Internal::OperatorTraits<Op>::getName() );

Here are the possible return values for OperatorTraits<Op>::getName():

template<Operator Op> struct OperatorTraits             { static const char* getName(){ return "*error*"; } };
template<> struct OperatorTraits<IsEqualTo>             { static const char* getName(){ return "=="; } };
template<> struct OperatorTraits<IsNotEqualTo>          { static const char* getName(){ return "!="; } };
template<> struct OperatorTraits<IsLessThan>            { static const char* getName(){ return "<"; } };
template<> struct OperatorTraits<IsGreaterThan>         { static const char* getName(){ return ">"; } };
template<> struct OperatorTraits<IsLessThanOrEqualTo>   { static const char* getName(){ return "<="; } };
template<> struct OperatorTraits<IsGreaterThanOrEqualTo>{ static const char* getName(){ return ">="; } };

Proposed Fix

(Available as PR: #618)

Remove the check and delete the default branch, which can never be reached:

diff --git a/include/internal/catch_result_builder.hpp b/include/internal/catch_result_builder.hpp
index d453fec..101f922 100644
--- a/include/internal/catch_result_builder.hpp
+++ b/include/internal/catch_result_builder.hpp
@@ -144,7 +144,7 @@ namespace Catch {
             return m_exprComponents.lhs.empty() ? m_assertionInfo.capturedExpression : m_exprComponents.op + m_exprComponents.lhs;
         else if( m_exprComponents.op == "matches" )
             return m_exprComponents.lhs + " " + m_exprComponents.rhs;
-        else if( m_exprComponents.op != "!" ) {
+        else {
             if( m_exprComponents.lhs.size() + m_exprComponents.rhs.size() < 40 &&
                 m_exprComponents.lhs.find("\n") == std::string::npos &&
                 m_exprComponents.rhs.find("\n") == std::string::npos )
@@ -152,8 +152,6 @@ namespace Catch {
             else
                 return m_exprComponents.lhs + "\n" + m_exprComponents.op + "\n" + m_exprComponents.rhs;
         }
-        else
-            return "{can't expand - use " + m_assertionInfo.macroName + "_FALSE( " + m_assertionInfo.capturedExpression.substr(1) + " ) instead of " + m_assertionInfo.macroName + "( " + m_assertionInfo.capturedExpression + " ) for better diagnostics}";
     }

 } // end namespace Catch

[philsquared]

Good catch, @RossBencina. Sorry it's taken a while to get to this (and thanks, @horenmar, for surfacing it in your review).

You are quite correct that the condition is never met.
However I believe this is because some other change altered that code path that would have led to it - and I'm not sure that's what we want.

The idea here is that if you write an expression, like:

REQUIRE( !thisShouldAlwaysBeFalse() );

• this is the natural way you'd probably write it - but we can't decompose that form. So that case was meant to have been detected at this point and the warning issued, so you might rewrite it as:

REQUIRE_FALSE( thisShouldAlwaysBeFalse() );

or:

REQUIRE( thisShouldAlwaysBeFalse() == false );

instead.

As I say, somewhere along the way that detection has gone missing. It's not behaviour-breaking so it looks like that got missed at the time. Maybe it's not a big deal. Perhaps it's not worth warning about this. What do you guys think?

If it's not worth warning about then let's go with Ross' PR (or something similar - I haven't checked it but I'm sure it's fine).

If we want to restore the original behaviour I'll take a look at where that needs to be added back in (shouldn't be a big job).

[RossBencina]

@philsquared If the code really is !thisShouldAlwaysBeFalse(), writing thisShouldAlwaysBeFalse() == false will not give much benefit to the user when the expression is decomposed and printed. On the other hand !(getSomething() == 42) would be better written in a way that allows it to be decomposed. I guess that printing the warning makes some sense. I'm not sure which way is better though.

[horenmar]

@RossBencina I think it was meant for objects that have overloaded conversion to bool, so that if you have an object that can be truthy or falsy, then for testing whether its falsy REQUIRE( !instance ) is fairly natural but prevents decomposition. On the other hand, REQUIRE_FALSE( instance ) would allow decomposition, but might not be obvious.

As to whether the warning is needed, I am not sure. I think it will be obvious to user when the decomposition fails, but it might not be so obvious that the proper fix is to use REQUIRE_FALSE instead of REQUIRE.

I think it would be worthwhile to make it work again, as long as the amount of work is not excessive.

[lightmare]

This (and the related PR) can be closed as the "impossible value" check has been knowingly removed as part of the lazy-reconstruction patch.