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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thanks for the comments. I posted an update for the simpler issues, working on refactoring the test cases and creating a fixit. Thanks for the good and actionable review comments!



================
Comment at: clang/docs/DiagnosticsReference.rst:2853
 
+-Wcompare-no-parentheses
+--------------------------------
----------------
Quuxplusone wrote:
> s/-no-/-op-/
fixed, thank you!


================
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`_.
 
----------------
Quuxplusone wrote:
> s/-no-/-op-/
> And what's going on with all these trailing underscores? If they're important, you're missing one.
missed that, thank you!


================
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;
+
----------------
Quuxplusone wrote:
> 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.
Thanks Arthur. I modeled the warning message after gcc's warning message. We found internally that while gcc detected this, clang did not. See https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options ...
<q>
-Wparentheses
Warn if parentheses are omitted in certain contexts, such as when there is an assignment in a context where a truth value is expected, or when operators are nested whose precedence people often get confused about.

Also warn if a comparison like x<=y<=z appears; this is equivalent to (x<=y ? 1 : 0) <= z, which is a different interpretation from that of ordinary mathematical notation.
...
</q>
While this is the gcc documentation, we can craft whatever message we see fit at this point in time :)  I'll add "chained" comparison for this next update, we can tailor the message as we see fit. Thanks!



================
Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+      << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+                     Self.PDiag(diag::note_precedence_silence)
----------------
lebedev.ri wrote:
> Should we also suggest the fix to rewrite into what user likely intended?
> `(x op1 y) && (y op2 z)`
I'll work on this, post in a next update. Thank you!


================
Comment at: clang/test/Sema/warn-compare-op-parentheses.c:1
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
----------------
Quuxplusone wrote:
> 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.
I'll address in a future update. Thank you!


================
Comment at: clang/test/Sema/warn-compare-op-parentheses.c:129
+  return 0;
+}
----------------
Quuxplusone wrote:
> 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
Great suggestions, I'll address in a future update. Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85097/new/

https://reviews.llvm.org/D85097