[PATCH] D85097: [Sema] add warning for comparisons like 'x<=y<=z'

[Arthur O'Dwyer via Phabricator via cfe-commits]

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