Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parentheses around macro argument "expr" #848

Closed
wants to merge 1 commit into from
Closed

Add parentheses around macro argument "expr" #848

wants to merge 1 commit into from

Conversation

Viatorus
Copy link

@Viatorus Viatorus commented Mar 9, 2017

Without paranteses arround the macro argument "expr" the used overloaded operator, the execution order and the resulting type is not clear for all macro substitutions:

Example:

expr = a || b:

// Bad:
(__catchResult <= expr ) -> (__catchResult <= a || b )

// Good:
(__catchResult <= (expr) ) -> (__catchResult <= (a || b) )

On GCC I get the warning without parentheses:
warning: suggest parentheses around comparison in operand of ‘==’ [-Wparentheses]

@Viatorus Viatorus changed the title Without paranteses arround the macro argument "expr" the used overloaded operator, the execution order and the resulting type is not clear for all macro substitutions: Add parantheses arround macro argument "expr" Mar 9, 2017
@Viatorus Viatorus changed the title Add parantheses arround macro argument "expr" Add parentheses arround macro argument "expr" Mar 9, 2017
@Viatorus Viatorus changed the title Add parentheses arround macro argument "expr" Add parentheses around macro argument "expr" Mar 9, 2017
@philsquared
Copy link
Collaborator

Could you give an example of what a and b might be?
Catch doesn't support composing expressions that like in REQUIRE or CHECK assertions (Matchers can be composed, though).
Adding parentheses around the expression would defeat the expression decomposition logic that allows lhs and rhs values to be reported independently.
You can, of course, add them in yourself at point of use, e.g.:

REQUIRE( (a || b) );

(But, again, you will lose the fine grained reporting).

@Viatorus
Copy link
Author

Viatorus commented Mar 9, 2017

I am not quite sure what you mean with:

you will lose the fine grained reporting

What do you lose exactly when you put a parentheses around expr?

I will leave an example asap.

@philsquared
Copy link
Collaborator

int a = 6*9;
REQUIRE( a == theAnswer() );

The failure output will tell you that the expression expanded to 54 == 42.

REQUIRE( ( a == theAnswer() ) );

With the extra parentheses the failure output will tell you that the expression expanded to false.

@Viatorus
Copy link
Author

Viatorus commented Mar 9, 2017

Oh, sorry - you are absolutely right, I didn't see it. So my fix is totally wrong. Sorry for this!

But the warning still exists:

#define CATCH_CONFIG_MAIN
#include "catch.hpp"

template<typename T>
void test1() {
    CHECK(T{} == false);
}

template<typename T>
void test2() {
    T v{};
    CHECK(v == false);
}

template<typename T>
void test3() {
    T v{};
    CHECK(v == v);
}

void test4() {
    using T = bool;
    T v{};
    CHECK(v == v);
}

void test5() {
    bool v{};
    CHECK(v == v);
}

TEST_CASE("Test leaf with scalar types") {
    test1<bool>();
    test2<bool>();
    test3<bool>();
    test4();
    test5();
};

test1, test2 and test3 raise a warning. And this is very annoying because warnings are errors for us...

Please see:
http://melpon.org/wandbox/permlink/VR75fS8EC905uloQ

Seems to be a gcc bug... or?

@horenmar
Copy link
Member

horenmar commented Mar 9, 2017

Yeah, there is a problem with warning suppression inside templated tests, that I am not quite sure how to fix.

Basically, if you are on a new-enough gcc (4.8+), then we use suppression based on _Pragma, which lets us suppress this warning for a single line, leaving it intact for the rest of the test file. For some reason, this does not work inside templates and I haven't gotten around investigating whether this is a gcc or standard bug (weirdness).

A solution is to do what we do for old gccs, just use pragma to disable the warning for the whole file.

@philsquared
Copy link
Collaborator

I'm going to close this PR as we've established it is not applicable.
The remaining issue is being tracked in #565 (although I'm not sure what, if anything, we can do about that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants