dexonsmith added a comment.
Did you miss this comment from my previous review?
> I would like to see tests like the following:
>
> NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning.
> NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning.
================
Comment at: lib/Sema/SemaExpr.cpp:12254-12255
+ if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansionLoc) &&
+ !SM.getImmediateMacroCallerLoc(LHSExpr->getExprLoc()).isMacroID() &&
+ OpExpansionLoc == ExprExpansionLoc)
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
----------------
Can you reverse these two checks? The second one looks cheaper.
================
Comment at: lib/Sema/SemaExpr.cpp:12243
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ } else {
+ SourceLocation OpExpansionLoc;
----------------
dexonsmith wrote:
> You can reduce nesting below by adding an early return above this.
>
> I also think you should describe at a high-level what you're trying to do in
> the code that follows. Something like:
> ```
> // Only diagnose operators in macros if they're from the same argument
> position.
> ```
You added an early return -- but then you didn't actually reduce the nesting.
Please remove the else that follows.
================
Comment at: test/Sema/parentheses.c:114
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
----------------
Higuoxing wrote:
> Higuoxing wrote:
> > dexonsmith wrote:
> > > Can you add fix-it CHECKs?
> > ```
> > llvm/tools/clang/test/Sema/parentheses.c:109:15: note: place parentheses
> > around the '&&' expression to silence this warning
> > VOID_CAST(i && i || i); // expected-warning {{'&&' within '||'}} \
> > ~~^~~~
> > llvm/tools/clang/test/Sema/parentheses.c:17:34: note: expanded from macro
> > 'VOID_CAST'
> > #define VOID_CAST(cond) ( (void)(cond) )
> > ^~~~
> > ```
> >
> > Sorry, it seems that when deal with expressions in macros, there is no
> > fix-it hint ...
> The `suggestParentheses` suppress the fix-it hint when the expression is in
> macros
>
> ```
> if (ParenRange.getBegin().isFileID() && ParenRange.getEnd().isFileID() &&
> EndLoc.isValid()) {
> Self.Diag(Loc, Note)
> << FixItHint::CreateInsertion(ParenRange.getBegin(), "(")
> << FixItHint::CreateInsertion(EndLoc, ")");
> } else {
> // We can't display the parentheses, so just show the bare note.
> Self.Diag(Loc, Note) << ParenRange;
> }
> ```
>
> You see, there is a `isFileID()`
Can you make it work? The diagnostic was disabled because it was low quality:
no fix-it, and firing when it was not actionable. I'm not convinced we should
turn it back on until we can give a fix-it.
https://reviews.llvm.org/D47687
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
