Quuxplusone added inline comments.
================
Comment at: clang/docs/DiagnosticsReference.rst:2853
+-Wcompare-no-parentheses
+--------------------------------
----------------
s/-no-/-op-/
================
Comment at: clang/docs/DiagnosticsReference.rst:9885
-Also controls `-Wbitwise-conditional-parentheses`_,
`-Wbitwise-op-parentheses`_, `-Wdangling-else`_, `-Wlogical-not-parentheses`_,
`-Wlogical-op-parentheses`_, `-Woverloaded-shift-op-parentheses`_,
`-Wparentheses-equality`_, `-Wshift-op-parentheses`_.
+Also controls `-Wbitwise-conditional-parentheses`_,
`-Wbitwise-op-parentheses`_, `-Wcompare-no-parentheses`, `-Wdangling-else`_,
`-Wlogical-not-parentheses`_, `-Wlogical-op-parentheses`_,
`-Woverloaded-shift-op-parentheses`_, `-Wparentheses-equality`_,
`-Wshift-op-parentheses`_.
----------------
s/-no-/-op-/
And what's going on with all these trailing underscores? If they're important,
you're missing one.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6135
+ "comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'">,
+ InGroup<CompareOpParentheses>, DefaultIgnore;
+
----------------
Why is this `x<=y<=z` instead of the simpler `x<y<z` or even `x<=y<z` (the
"half-open range" common case)?
IMHO you should mention the name "chained comparisons" here, since I think
that's what people coming from such languages will understand.
================
Comment at: clang/test/Sema/warn-compare-op-parentheses.c:1
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
----------------
These are some great test cases, but I think they would benefit from being
exhaustive.
```
int tests(int p1, int p2, int p3) {
bool b;
b = (p1 < p2 < p3); // expected-warning{{comparisons like 'x<y<z' are
interpreted}} expected-note{{place parentheses}}
b = (p1 < p2 <= p3); // expected-warning{{comparisons like 'x<y<z' are
interpreted}} expected-note{{place parentheses}}
b = (p1 < p2 > p3); // expected-warning{{comparisons like 'x<y<z' are
interpreted}} expected-note{{place parentheses}}
b = (p1 < p2 >= p3); // expected-warning{{comparisons like 'x<y<z' are
interpreted}} expected-note{{place parentheses}}
b = (p1 <= p2 < p3); // expected-warning{{comparisons like 'x<y<z' are
interpreted}} expected-note{{place parentheses}}
b = (p1 <= p2 <= p3); // expected-warning{{comparisons like 'x<y<z' are
interpreted}} expected-note{{place parentheses}}
```
etc. etc.
I don't see any downside to being systematic here.
================
Comment at: clang/test/Sema/warn-compare-op-parentheses.c:129
+ return 0;
+}
----------------
I would like to see explicit (and preferably exhaustive or at least systematic)
test cases for the "no warning intended" case:
if ((p1 < p2) < p3)
if (p1 < (p2 < p3))
if (0 <= (p1 < p2)) // this should already trigger a
constant-comparison-result warning, yes?
I would like to see explicit (and preferably exhaustive or at least systematic)
test cases for mixed relational and equality comparisons:
if (p1 == p2 < p3) // maybe intentional, but definitely deserving of a
warning
if (p1 == p2 == p3) // definitely buggy and deserving of a warning
if (p1 != p2 != p3) // definitely buggy and deserving of a warning
if (p1 < p2 == p3 < p4) // maybe intentional, but definitely deserving of
a warning
if (p1 == p2 < p3 == p4) // definitely buggy and deserving of a warning
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85097/new/
https://reviews.llvm.org/D85097
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
