[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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