Expression is evaluated twice when operator&&(bool, decltype(expr)) is defined

[hpesoj]

} while( Catch::isTrue( false && (expr) ) )

This line assumes the built-in && is used. If a user-defined operator&& is used, the expression is not short-circuited, and expr is evaluated here (and twice overall). I know that overloading && is not generally recommended, but I think this corner-case can be fixed by explicitly casting expr to bool:

} while( Catch::isTrue( false && static_cast<bool>(expr) ) )

[philsquared]

I never got back to you about this.
I implemented your suggested fix - but it turns out that has an unwanted side-effect (see #657).
I'm considering using !!(expr) instead. That seems to do the trick for me but I'd appreciate it you were able to confirm that it has the desired effect for you, too?

[philsquared]

Actually I'm fairly confident that this should work for you. I've pushed a commit with the change in.
Please do let me know if it causes any problems.

[hpesoj]

Hi Phil. I suggested the original fix for compatibility with a custom
boolean type which overloaded the && operator. Said type also overloaded
the ! operator, so your proposed change would reintroduce the
incompatibility for such types. I believe keeping both the !! and the
static cast would fix both issues, though I am not 100% sure if other
issues wouldn't arise.

However, I have since removed the && operator overload for my type as I
feel short circuiting evaluation is too useful a feature to sacrifice.
Thus, your proposed change would no longer affect me personally. Perhaps
you feel that it is not worth attempting to support such a corner case?
On 13 May 2016 02:12, "Phil Nash" notifications@github.com wrote:

I never got back to you about this.
I implemented your suggested fix - but it turns out that has an unwanted
side-effect (see #657 #657).
I'm considering using !!(expr) instead. That seems to do the trick for me
but I'd appreciate it you were able to confirm that it has the desired
effect for you, too?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#574 (comment)

[philsquared]

The problem with the && overload was that short circuiting was disabled - and the whole point of this expression was for it to be seen by the compiler (so you get natural warnings etc) without being evaluated at runtime.
Using !! puts us back into the realm of short-circuiting, so the original expression should not be evaluated at runtime.
At that point as long as the ! operator compiles for your type (and, ideally, doesn't generate any warnings) then it shouldn't matter what it actually does - it cannot influence short-circuiting here so should never be executed.

I put together a little test case for this which reproduced your original issue when I didn't use the cast or the !! - but had no issues (with && or !) with the !! approach.

So I'm inclined to leave things as they are here. If anyone else hits problems with it I'll consider reverting back to the original, though.

Thanks for taking the time to respond - especially given that the original issue no longer applies to you!

[hpesoj]

The problem is that my type overloads the ! operator to produce an object
of the same type, rather than a bool. Given that the aim of the !!
expression is to convert an object to a bool (without generating warnings -
which I overlooked with my original suggestion), I don't think it will work
correctly with such behaviour. Double negating followed by a static cast
should account for all (sensibly behaved - if you consider overloading the
&& operator sensible) combinations of operator overloads.
On 13 May 2016 14:35, "Phil Nash" notifications@github.com wrote:

The problem with the && overload was that short circuiting was disabled -
and the whole point of this expression was for it to be seen by the
compiler (so you get natural warnings etc) without being evaluated at
runtime.
Using !! puts us back into the realm of short-circuiting, so the original
expression should not be evaluated at runtime.
At that point as long as the ! operator compiles for your type (and,
ideally, doesn't generate any warnings) then it shouldn't matter what it
actually does - it cannot influence short-circuiting here so should never
be executed.

I put together a little test case for this which reproduced your original
issue when I didn't use the cast or the !! - but had no issues (with &&
or !) with the !! approach.

So I'm inclined to leave things as they are here. If anyone else hits
problems with it I'll consider reverting back to the original, though.

Thanks for taking the time to respond - especially given that the original
issue no longer replies to you!

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#574 (comment)

[horenmar]

I tested this recently with a custom class

struct tester {
    tester& operator&&(const tester& rhs) {
        std::cout << "Operator&&\n";
        return *this;
    }

    tester& operator!() {
        std::cout << "Operator!\n";
        return *this;
    }

    tester& operator()() {
        std::cout << "Operator()\n";
        return *this;
    }

    explicit operator bool() const {
        std::cout << "Bool conversion\n";
        return true;
    }
};

with tests

TEST_CASE("Custom &&") {
    tester t1, t2;
    REQUIRE((t1 && t2()));
    REQUIRE(t1);
    REQUIRE(t1());
}

and couldn't reproduce the problem. If no-one can confirm it still exists, I am going to close it.

[philsquared]

Ah but it is reproducible if you take out the explicit operator bool

[philsquared]

although I'm not sure how this is meant to be used in an assert if there's no way to interpret it as a bool.
I've confirmed that no warning arise with (implicit) operator bool too.

If @hpesoj is still watching perhaps he can comment on how well @horenmar's example captures the problem (or if, indeed, there is still a problem).

[lightmare]

I think what @hpesoj meant was that (false && (expr)) will use this operator:

zzyzx operator && (bool lhs, const tester& rhs);

if it exists, unless you force the conversion of (expr) to bool

[philsquared]

Originally, yes, @lightmare.
But when it was changed to !!(expr) he then said that his type overloaded operator ! too, and so still had the issue. @horenmar's example attempted to capture that - but only does so without an operator bool (whether implicit or explicit) - at least in my testing.
Without conversion to bool or an operator ! that returns bool, I'm not sure how it could be used in an assertion in the first place, so I'm hoping @hpesoj can shed some light on it.

[lightmare]

Well, when a user writes

Foo foo;
CHECK(foo);

we can safely assume foo is convertible to bool; otherwise the macro can expand to ill-formed code just like if (foo); would be ill-formed. My point was that when there is a

X operator && (bool a, const Foo& b);

it will be selected by overload resolution, because it doesn't require any user-defined conversion. The compiler will not convert foo to bool (not even when Foo::operator bool is non-explicit, it's still user-defined conversion) and then built-in short-circuiting &&.

[lightmare]

In other words, false && !!(expr) is not the right way to work-around overloaded operator &&, because operator ! is not guaranteed to return bool.

false && static_cast<bool>(expr) is the correct solution. The static_cast mimics if (expr) -- it invokes implicit or explicit conversion, and the && is then guaranteed to have a bool on both sides so short-circuits.

[horenmar]

@lightmare Problem with static_cast<bool> is that it triggers (dumb) MSVC warning, C4800... This means that Catch is going to end up with this beauty: static_cast<bool>( !!(expr) ).

I also managed to send current master into infinite loop: (Compiled with VS2015, Update 3)

struct from {
    from& operator!() {
        std::cout << "Operator!\n";
        return *this;
    }

    operator bool() const {
        std::cout << "Bool conversion -- from\n";
        return true;
    }
};

struct to {
    operator bool() const {
        std::cout << "Bool conversion -- to\n";
        return true;
    }
};

to operator&&(bool lhs, const from& rhs) {
    std::cout << "operator&&\n";
    return{};
}

TEST_CASE("Custom &&") {
    from f;
    REQUIRE(f);
}

[hpesoj]

Yes, false && static_cast<bool>( !!(expr) ) was the fix I suggested. I think this should force conversion to bool and suppress the VC++ warning.

[lightmare]

I should've asked this much earlier, but what exactly is the purpose of "it forces the compiler to give it a look"?

In the INTERNAL_CATCH_TEST macro with unary expr, ExpressionLhs::endExpression() already does "give it a look" by way of value = (expr) ? true : false. Which by the way is another way to convert to bool, one that I hope MSVC wouldn't complain about.

So that suggests another alternative without introducing more overloaded operators:

false && ((expr) ? true : false)
// and another that just occurred to me along the way
false && sizeof(expr)

but I wonder why any of this would be needed in the first place... 😖

[philsquared]

@hpesoj thanks - so you do have a bool operator? (explicit or implicit?).

@lightmare the expression that gets evaluated is the one that is decomposed by Catch, then reassembled. Because of the way some literals (particularly integers) are captured by template, along with some of the machinery in Catch to deal with that, the expression that is actually evaluated on reassembly may not be exactly the same. So to make sure any warnings that might be emitted for the actual expression that are not for the reconstucted expression this technique forces the compiler to also evaluated the original expression as well.

There is another way I was toying with:

bool exprAsBool( T const& ){ return false; }
// ..
Catch::isTrue( false && exprAsBool( expr )

But I think I like the sizeof one even more (with one small change - compare the result to another integer to avoid int-conversion-to-bool warnings)!

[hpesoj]

My type was modelling a boolean value, so it had an implicit operator bool.

[lightmare]

But I think I like the sizeof one even more (with one small change - compare the result to another integer to avoid int-conversion-to-bool warnings)!

Right, I realized that after posting, and also that the false && would then be unnecessary. I wasn't sure whether just

} while ( sizeof(expr) < 0 );

won't produce another warning; tried that with gcc and although unsigned_var < 0 triggers -Wtype-limits, sizeof(x) < 0 doesn't. If it does produce warning with another compiler, then this should hopefully silence it:

} while ( exprAsBool( sizeof(expr) ) );
// I'd rename exprAsBool => alwaysFalse