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

[Xing via Phabricator via cfe-commits]

Higuoxing marked 9 inline comments as done.
Higuoxing added inline comments.


================
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) )
+
----------------
dexonsmith wrote:
> - You haven't actually wrapped `NESTING_VOID` here.
> - A more descriptive name would be `APPLY_OPS` or something.
Yes, I made a crucial mistake! So, I make a little adjustment here, we will check parentheses for expressions, if and only if the operator and its LHS, RHS are from same arg position of a *non-nesting macro*, because it seems very difficult to distinguish a nesting macro's args are from same arg position of its `father` macro


================
Comment at: test/Sema/parentheses.c:109-111
+  //===----------------------------------------------------------------------
+  // Logical operator in macros
+  //===----------------------------------------------------------------------
----------------
dexonsmith wrote:
> I'm not sure this comment is particularly useful.
Yes, I delete them : )


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


================
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)
----------------
dexonsmith wrote:
> 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.
Yes, I delete them


https://reviews.llvm.org/D47687