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

Resolved #359 - Eclipse CDT - Method 'endExpression' could not be resolved #393

Closed
wants to merge 3 commits into from

Conversation

kentsangkm
Copy link

Eclipse indexer did not handle the ->* operator correctly and display error in the editor, whereas it actually is not a bug. Changing the operator from operator->* to operator+ can resolve this problem.

Proof of concept:
Priority 3 operator+ Right-to-left
Priority 4 operator->* Left-to-right

where:
__catchResult+expr

Supported operator in expr:
Prority 3 +
Prority 3 -
Priority 3 /
Priority 3 *
Priority 13 && *STATIC_ASSERT_Expression_Too_Complex_Please_Rewrite_As_Binary_Comparison
Priority 14 || *STATIC_ASSERT_Expression_Too_Complex_Please_Rewrite_As_Binary_Comparison

__catchResult + _1Obj operand1 _2Obj .... _NObj
=> (__catchResult + _1Obj) operand1 _2Obj .... _NObj
=> ExpressionLhs operand1 _2Obj .... _NObj
Hence, the change is OK

kentsangkm added 3 commits March 21, 2015 16:49
Eclipse header indexer did not handle the operator->* correctly. So
rename the function from operator->* to eval can resolve the Eclipse
false alert.
This reverts commit c97927f.
Eclipse indexer did not handle the ->* operator correctly and display
error in the editor, whereas it actually is not a bug. Changing the
operator from operator->* to operator+ can resolve this problem.

Proof of concept:
Prority 3   operator+       Right-to-left
Prority 4   operator->*     Left-to-right

where:
__catchResult+expr

Supported operator in expr:
Prority 3   +
Prority 3   -
Prority 3   /
Prority 3   *
Prority 13   &&
*STATIC_ASSERT_Expression_Too_Complex_Please_Rewrite_As_Binary_Comparison
Prority 14   ||
*STATIC_ASSERT_Expression_Too_Complex_Please_Rewrite_As_Binary_Comparison

__catchResult + _1Obj   operand1 _2Obj .... _NObj
=> (__catchResult + _1Obj) operand1 _2Obj .... _NObj
=> ExpressionLhs<T>        operand1 _2Obj .... _NObj
Hence, the change is OK
@chvillanuevap
Copy link

I tried this and it worked on Eclipse Luna.

@RomainGoussault
Copy link

Same here, I tried this and it worked on Eclipse Luna, Ubuntu

@Syndog
Copy link

Syndog commented Jun 15, 2015

This worked great for me in Windows/Luna. Thank you! Hope it gets pulled into master soon.

@PureAbstract
Copy link
Contributor

This seems to be a workaround for what appears to be a bug in Eclipse - has anyone raised a bug with the Eclipse folks?

@ghost
Copy link

ghost commented Jun 16, 2015

works on Eclipse Luna, Ubuntu:

  • Eclipse IDE for C/C++ Developers, Version: Luna Service Release 2 (4.4.2), Build id: 20150219-0600
  • Ubuntu 14.04.2 LTS (GNU/Linux 3.13.0-54-generic x86_64)

@Syndog
Copy link

Syndog commented Jun 16, 2015

@philsquared
Copy link
Collaborator

As @PureAbstract says, this is a workaround for an IDE shortcoming. That's not to dismiss it - but there are some other consequences to such a change.
The reason ->* was chosen originally was because it has the highest precedence of all suitable operators (overridable, left-to-right associative and no trailing brackets). Well, joint highest, along with .*.
+ (and - for that matter) have lower precedence than, say, * and /. As it turns out this is ok, because all these operators have higher precedence than the comparison operators - so any lhs expressions involving * and / will be evaluated before binding to the decomposer.
However now there is an inconsistency! We can use * and / on the lhs, but not + or -. Is this a big deal? Probably not - but it's a little irksome. And it opens up the question: can we chose an operator that will allow + and - to be evaluated too?
The answer is yes. operator << has higher precedence than + and - - but still lower than the comparison operators. This appears to work. It has one issue that I'm currently debating with myself whether it's enough to nix it or not (thoughts welcome!). The issue is that, with clang, at least - with the warning levels I have - I get a warning about overloaded operator << having lower precedence than the comparison operators!
The trouble is I can't (AFAIK) suppress that warning locally to the source because you can't emit #pragmas within a #define - so I'd have to suppress the warning for all test code - which I don't like doing if I can help it.

Alternatively, one of the non-equality comparison operators would also do the trick (e.g. <=), and this has been suggested before (#247). This works because all four operators have the same precedence so will still bind most tightly to the left.
I don't get that warning when I use <= but the last couple of comments in the referenced PR suggest that there were warnings being seen at the time. I'm not sure if that's differences in the compiler or the rest of the Catch code (it was a while ago). If anyone has any input on that I'd be grateful to hear.

If there are warnings with <= then the option suggested here (in the PR) is probably a good enough compromise, after all. It works around the Eclipse issue. It allows *, / (and %) to be used on the LHS, but doesn't trigger any warnings.

Thoughts?

@martinmoene
Copy link
Collaborator

The trouble is I can't (AFAIK) suppress that warning locally to the source because you can't emit #pragmas within a #define - so I'd have to suppress the warning for all test code - which I don't like doing if I can help it.

perhaps via _Pragma(...) ?

http://stackoverflow.com/questions/3030099/pragma-in-define-macro

@philsquared
Copy link
Collaborator

Well well. I didn't know about that one. Thanks @martinmoene!

_Pragma( "clang diagnostic ignored \"-Wparentheses\"" )

works for me in clang. I'm not sure how to use it portably, though. It's now got incompatibilities on two axes (_Pragma is C++11, AFAICS - and then the pragma itself is clang specific)!

@martinmoene
Copy link
Collaborator

But do other compilers have the problem that calls for the pragma?
If not, could you #define _Pragma( sink ) /*empty*/ before and #undef _Pragma after your macro ?

Hold on, scrap the second line, it has to be more sophisticated than that...

#if CATCH_COMPILER_IS_CLANG
# define CATCH_EXPR_PRAGMA _Pragma( "clang diagnostic ignored \"-Wparentheses\"" )
#else
# define CATCH_EXPR_PRAGMA
#endif

#define EXPR( expr ) \
    CATCH_EXPR_PRAGMA \
    expr

EXPR( 1 < 2 )

// g++ -E -DCATCH_COMPILER_IS_CLANG main.cpp

philsquared added a commit that referenced this pull request Jun 29, 2015
Allows more complex LHS expressions - and works around an Eclipse bug.
See Issues #359, #393 and #247 for details
martinmoene added a commit to martinmoene/lest that referenced this pull request Jul 22, 2015


This enables the use of *, /, %, + and - at the LHS of expressions used in assertion macros, e.g. EXPECT( 1 + 2 == 3 )

See also discussion at
- Catch commit: Use <= operator instead of ->* for decomposer,
  catchorg/Catch2@8cc1108 and issues mentioned there:
- Catch issue #359, catchorg/Catch2#359
- Catch issue #393, catchorg/Catch2#393
- Catch issue #247, catchorg/Catch2#247
@kentsangkm kentsangkm closed this Sep 22, 2016
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.

7 participants