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

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

dexonsmith added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:12236
 
+/// Look for '&&' in the righ or left hand of a '||' expr
+static void DiagnoseLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc,
----------------
- Please add a period at the end of the sentence.
- "righ" should be "right".



================
Comment at: lib/Sema/SemaExpr.cpp:12243
+    DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+  } else {
+    SourceLocation OpExpansionLoc;
----------------
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.
```


================
Comment at: lib/Sema/SemaExpr.cpp:12249
+    SourceLocation ExprExpansionLoc;
+    // LHS and operator are from same arg position of macro function
+    if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansionLoc) &&
----------------
This comment is just describing what's clear from the code.  I think you should drop it, and the similar one later.


================
Comment at: lib/Sema/SemaExpr.cpp:12251
+    if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansionLoc) &&
+                                OpExpansionLoc == ExprExpansionLoc)
+      DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
----------------
This line has odd indentation.  Please run clang-format-diff.py to clean up the patch.


================
Comment at: test/Sema/parentheses.c:17-18
 
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
----------------
I don't think these comments are useful.


================
Comment at: test/Sema/parentheses.c:19
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
----------------
Can you combine this with `NON_NESTING_VOID_0` (which I think should be called `VOID_CAST`) below?


================
Comment at: test/Sema/parentheses.c:20
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
----------------
- You haven't actually wrapped `NESTING_VOID` here.
- A more descriptive name would be `APPLY_OPS` or something.


================
Comment at: test/Sema/parentheses.c:23
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
----------------
This name is strange.  `VOID_CAST` would be more descriptive.


================
Comment at: test/Sema/parentheses.c:24
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
----------------
I suggest `APPLY_OPS_DIRECTLY`.


================
Comment at: test/Sema/parentheses.c:109-111
+  //===----------------------------------------------------------------------
+  // Logical operator in macros
+  //===----------------------------------------------------------------------
----------------
I'm not sure this comment is particularly useful.


================
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.
----------------
Can you add fix-it CHECKs?


================
Comment at: test/Sema/parentheses.c:117-124
+  // NON_NESTING_VOID_1(&&, ||, i, i, i);
+  // will be expanded to:
+  // i && i || i
+  // ^ arg position 2 (i)
+  //    ^ arg position 0 (&&)
+  //      ^ arg position 3 (||) should not be checked becaues `op ||` and nothing from same arg position
+  //         ^ arg position 1 (i)
----------------
I think this comment should be fairly well implied by the commit and commit message the test is part of.  I don't think it's necessary.


================
Comment at: test/Sema/parentheses.c:140
+  
+  // same as this one
+  NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
----------------
Not a useful comment.


================
Comment at: test/Sema/parentheses.c:153
+  //                     ^ arg position 4 (i)
+  NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+                                                 // expected-note {{place parentheses around the '&&' expression to silence this warning}}
----------------
I don't think checking within other macro arguments is necessary here.  You have a combinatorial explosion of tests, but it seems unlikely code would be written in such a way as to make this wrong.

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.
```


https://reviews.llvm.org/D47687