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

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

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Sema/SemaExpr.cpp:12304-12309
   // Diagnose "arg1 & arg2 | arg3"
   if ((Opc == BO_Or || Opc == BO_Xor) &&
       !OpLoc.isMacroID()/* Don't warn in macros. */) {
     DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, LHSExpr);
     DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, RHSExpr);
   }
----------------
It seems unfortunate not to update this one at the same time.  Or are you planning that for a follow-up?

Can you think of a way to share the logic cleanly?


================
Comment at: lib/Sema/SemaExpr.cpp:12313
   // We don't warn for 'assert(a || b && "bad")' since this is safe.
-  if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
-    DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
-    DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+  if (Opc == BO_LOr) {
+    if (!OpLoc.isMacroID()) {
----------------
I think the code inside this should be split out into a separate function (straw man name: "diagnoseLogicalAndInLogicalOr") so that you can use early returns and reduce nesting.


================
Comment at: lib/Sema/SemaExpr.cpp:12315
+    if (!OpLoc.isMacroID()) {
+      // Operator is not in macros
+      DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
----------------
I don't think this comment is valuable.  It's just repeating the code in the condition.


================
Comment at: lib/Sema/SemaExpr.cpp:12319
+    } else {
+      // This Expression is expanded from macro
+      SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
----------------
This comment is just repeating what's in the condition.  I suggest replacing it with something describing the logic that follows.  Also, it's missing a period at the end of the sentence.


================
Comment at: lib/Sema/SemaExpr.cpp:12321
+      SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
+      if ((Self.getSourceManager().isMacroArgExpansion(OpLoc, 
+                                                      &OpExpansionLoc) &&
----------------
This will be cleaner if you create a local reference to `Self.getSourceManager()` called `SM`.


================
Comment at: lib/Sema/SemaExpr.cpp:12325-12328
+          (Self.getSourceManager().isMacroArgExpansion(OpLoc, 
+                                                      &OpExpansionLoc) &&
+          Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+                                                      &LHSExpansionLoc))) {
----------------
It's a little awkward to add this condition in with the previous one, and you're also repeating a call to `isMacroArgExpansion` unnecessarily.  I suggest something like:

```
SourceLocation OpExpansion;
if (!SM.isMacroArgExpansion(OpLoc, &OpExpansion))
  return;

SourceLocation ExprExpansion;
if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansion) &&
    OpExpansion == ExprExpansion)
  DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);

if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), &ExprExpansion) &&
    OpExpansion == ExprExpansion)
  DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
```



================
Comment at: test/Sema/parentheses.c:133
+                                                   // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+  NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
----------------
typo: should be "no warning".


https://reviews.llvm.org/D47687