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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
