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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
vabridgers requested review of this revision.

This changes add a new warning named -Wcompare-op-parentheses that's
part of the -Wparentheses diagnostic group. This diagnostic produces a
warning when a pattern like 'x<=y<=z' is found. When this pattern is not
qualified by parentheses, it's equivalent to '(x<=y ? 1 : 0) <= z',
which is a different interpretation from that of ordinary mathematical
notation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int case1(int p1, int p2, int p3) {
+  if (p1 < p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case2(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2 < p3))
+return 1;
+  return 0;
+}
+
+int case3(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2))
+return 1;
+  return 0;
+}
+
+int case4(int p1, int p2, int p3) {
+  // no warning
+  if ((p1) && (p3 < p2))
+return 1;
+  return 0;
+}
+
+int case5(int p1, int p2, int p3) {
+  while (p1 < p3 < p2)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case6(int p1, int p2, int p3) {
+  // should not warn
+  while (p1 && p3 < p2)
+return 1;
+  return 0;
+}
+
+int case7(int p1, int p2, int p3) {
+  // should not warn
+  while ((p1 < p3) < p2)
+return 1;
+  return 0;
+}
+
+int case8(int p1, int p2, int p3) {
+  int ret = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  return ret;
+}
+
+int case9(int p1, int p2, int p3) {
+  int ret = (p1 < p2) < p3 < p1;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  return ret;
+}
+
+int case10(int p1, int p2, int p3) {
+  if (p1 <= p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case11(int p1, int p2, int p3) {
+  if (p1 < p2 <= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case12(int p1, int p2, int p3) {
+  if (p1 <= p2 <= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case13(int p1, int p2, int p3) {
+  if (p1 > p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case14(int p1, int p2, int p3) {
+  if (p1 > p2 > p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case15(int p1, int p2, int p3) {
+  if (p1 >= p2 > p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case16(int p1, int p2, int p3) {
+  if (p1 >= p2 >= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case17(int p1, int p2, int p3) {
+  // should not warn
+  if (p1 >= p2 || p3)
+return 1;
+  return 0;
+}
Index: clang/test/Misc/warning-wall.c
===

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

This is an implementation in the CFE, after submitting and getting comments on 
https://reviews.llvm.org/D84898.


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

[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, DefaultIgnore;
+

Why is this `x<=y<=z` instead of the simpler `x p3);  // expected-warning{{comparisons like 'x= p3);  // expected-warning{{comparisons like 'xhttps://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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you! I strongly prefer this path forward.




Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

Should we also suggest the fix to rewrite into what user likely intended?
`(x op1 y) && (y op2 z)`


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

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

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

Quuxplusone added a comment.

Btw, this is an awesome patch! I'm looking forward to getting to use it.


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

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thank you for the comments @lebedev.ri and @Quuxplusone. I'll abandon the tidy 
approach (https://reviews.llvm.org/D84898) and work towards satisfying these 
review comments (and any others), driving towards acceptance. Best!


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

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 282471.
vabridgers marked 2 inline comments as done.
vabridgers added a comment.

Address simpler issues brought up during review so far.
Tests to be refactored, and a fixit added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int case1(int p1, int p2, int p3) {
+  if (p1 < p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case2(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2 < p3))
+return 1;
+  return 0;
+}
+
+int case3(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2))
+return 1;
+  return 0;
+}
+
+int case4(int p1, int p2, int p3) {
+  // no warning
+  if ((p1) && (p3 < p2))
+return 1;
+  return 0;
+}
+
+int case5(int p1, int p2, int p3) {
+  while (p1 < p3 < p2)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case6(int p1, int p2, int p3) {
+  // should not warn
+  while (p1 && p3 < p2)
+return 1;
+  return 0;
+}
+
+int case7(int p1, int p2, int p3) {
+  // should not warn
+  while ((p1 < p3) < p2)
+return 1;
+  return 0;
+}
+
+int case8(int p1, int p2, int p3) {
+  int ret = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  return ret;
+}
+
+int case9(int p1, int p2, int p3) {
+  int ret = (p1 < p2) < p3 < p1;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  return ret;
+}
+
+int case10(int p1, int p2, int p3) {
+  if (p1 <= p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case11(int p1, int p2, int p3) {
+  if (p1 < p2 <= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case12(int p1, int p2, int p3) {
+  if (p1 <= p2 <= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case13(int p1, int p2, int p3) {
+  if (p1 > p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case14(int p1, int p2, int p3) {
+  if (p1 > p2 > p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case15(int p1, int p2, int p3) {
+  if (p1 >= p2 > p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case16(int p1, int p2, int p3) {
+  if (p1 >= p2 >= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case17(int p1, int p2, int p3) {
+  // should not warn
+  if (p1 >= p2 || p3)
+return 1;
+  return 0;
+}
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -1,99 +1,9 @@
-RUN: diagtool tree -Wall > %t 2>&1
-RUN: FileCheck --input-file=%t %s
+RUN : diag

[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, DefaultIgnore;
+

Quuxplusone wrote:
> Why is this `x<=y<=z` instead of the simpler `x "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 ...

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

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 interpreted}} expected-note{{place parentheses}}
> b = (p1 < p2 <= p3);  // expected-warning{{comparisons like 'x interpreted}} expected-note{{place parentheses}}
> b = (p1 < p2 > p3);  // expected-warning{{comparisons like 'x interpreted}} expected-note{{place parentheses}}
> b = (p1 < p2 >= p3);  // expected-warning{{comparisons like 'x interpreted}} expected-note{{place parentheses}}
> b = (p1 <= p2 < p3);  // expected-warning{{comparisons like 'x interpreted}} expected-note{{place parentheses}}
> b = (p1 <= p2 <= p3);  // expected-warning{{comparisons like 'x 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 == 

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added inline comments.



Comment at: clang/test/Misc/warning-wall.c:6
+   
  CHECK -
+   
  NEXT : -Wchar - subscripts CHECK - NEXT : -Wcomment CHECK - NEXT : -Wdelete - 
non - virtual - dtor CHECK - NEXT : -Wdelete - non - abstract - non - virtual - 
dtor CHECK - NEXT : -Wdelete - abstract - non - virtual - dtor CHECK - NEXT : 
-Wformat CHECK - NEXT : -Wformat - extra - args CHECK - NEXT : -Wformat - zero 
- length CHECK - NEXT : -Wnonnull CHECK - NEXT : -Wformat - security CHECK - 
NEXT : -Wformat - y2k CHECK - NEXT : -Wformat - invalid - specifier CHECK - 
NEXT : -Wfor - loop - analysis CHECK - NEXT : -Wframe - address CHECK - NEXT : 
-Wimplicit CHECK - NEXT : -Wimplicit - function - declaration CHECK - NEXT : 
-Wimplicit - int CHECK - NEXT : -Winfinite - recursion CHECK - NEXT : -Wint - 
in - bool - context CHECK - NEXT : -Wmismatched - tags CHECK - NEXT : -Wmissing 
- braces CHECK - NEXT : -Wmove CHECK - NEXT : -Wpessimizing - move CHECK - NEXT 
: -Wredundant - move CHECK - NEXT : -Wreturn - std - move CHECK - NEXT : -Wself 
- move CHECK - NEXT : -Wmultichar CHECK - NEXT : -Wrange - loop - construct 
CHECK - NEXT : -Wreorder CHECK - NEXT : -Wreorder - ctor CHECK - NEXT : 
-Wreorder - init - list CHECK - NEXT : -Wreturn - type CHECK - NEXT : -Wreturn 
- type - c - linkage CHECK - NEXT : -Wself - assign CHECK - NEXT : -Wself - 
assign - overloaded CHECK - NEXT : -Wself - assign - field CHECK - NEXT : 
-Wself - move CHECK - NEXT : -Wsizeof - array - argument CHECK - NEXT : 
-Wsizeof - array - decay CHECK - NEXT : -Wstring - plus - int CHECK - NEXT : 
-Wtautological - compare CHECK - NEXT : -Wtautological - constant - compare 
CHECK - NEXT : -Wtautological - constant - out - of - range - compare CHECK - 
NEXT : -Wtautological - pointer - compare CHECK - NEXT : -Wtautological - 
overlap - compare CHECK - NEXT : -Wtautological - bitwise - compare CHECK - 
NEXT : -Wtautological - undefined - compare CHECK - NEXT : -Wtautological - 
objc - bool - compare CHECK - NEXT : -Wtrigraphs CHECK - NEXT : -Wuninitialized 
CHECK - NEXT : -Wsometimes - uninitialized CHECK - NEXT : -Wstatic - self - 
init CHECK - NEXT : -Wuninitialized - const - reference CHECK - NEXT : 
-Wunknown - pragmas CHECK - NEXT : -Wunused CHECK - NEXT : -Wunused - argument 
CHECK - NEXT : -Wunused - function CHECK - NEXT : -Wunneeded - internal - 
declaration CHECK - NEXT : -Wunused - label CHECK - NEXT : -Wunused - private - 
field CHECK - NEXT : -Wunused - lambda - capture CHECK - NEXT : -Wunused - 
local - typedef CHECK - NEXT : -Wunused - value CHECK - NEXT : -Wunused - 
comparison CHECK - NEXT : -Wunused - result CHECK - NEXT : -Wunevaluated - 
expression CHECK - NEXT : -Wpotentially - evaluated - expression CHECK - NEXT : 
-Wunused - variable CHECK - NEXT : -Wunused - const - variable CHECK - NEXT : 
-Wunused - property - ivar CHECK - NEXT : -Wvolatile - register - var CHECK - 
NEXT : -Wobjc - missing - super - calls CHECK - NEXT : -Wobjc - designated - 
initializers CHECK - NEXT : -Wobjc - flexible - array CHECK - NEXT : 
-Woverloaded - virtual CHECK - NEXT : -Wprivate - extern CHECK - NEXT : -Wcast 
- of - sel - type CHECK - NEXT : -Wextern - c - compat CHECK - NEXT : -Wuser - 
defined - warnings CHECK - NEXT : -Wparentheses CHECK - NEXT : -Wcompare - op - 
parentheses CHECK - NEXT : -Wlogical - op - parentheses CHECK - NEXT : 
-Wlogical - not -parentheses CHECK - NEXT : -Wbitwise - conditional - 
parentheses CHECK - NEXT : -Wbitwise - op - parentheses CHECK - NEXT : -Wshift 
- op - parentheses CHECK - NEXT : -Woverloaded - shift - op - parentheses CHECK 
- NEXT : -Wparentheses - equality CHECK - NEXT : -Wdangling - else CHECK - NEXT 
: -Wswitch CHECK - NEXT : -Wswitch - bool CHECK - NEXT : -Wmisleading - 
indentation
 

oh my, I didn't intend for this to happen. I'll address.


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

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 282475.
vabridgers added a comment.

back out last unwanted changes from clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int case1(int p1, int p2, int p3) {
+  if (p1 < p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case2(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2 < p3))
+return 1;
+  return 0;
+}
+
+int case3(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2))
+return 1;
+  return 0;
+}
+
+int case4(int p1, int p2, int p3) {
+  // no warning
+  if ((p1) && (p3 < p2))
+return 1;
+  return 0;
+}
+
+int case5(int p1, int p2, int p3) {
+  while (p1 < p3 < p2)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case6(int p1, int p2, int p3) {
+  // should not warn
+  while (p1 && p3 < p2)
+return 1;
+  return 0;
+}
+
+int case7(int p1, int p2, int p3) {
+  // should not warn
+  while ((p1 < p3) < p2)
+return 1;
+  return 0;
+}
+
+int case8(int p1, int p2, int p3) {
+  int ret = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  return ret;
+}
+
+int case9(int p1, int p2, int p3) {
+  int ret = (p1 < p2) < p3 < p1;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  return ret;
+}
+
+int case10(int p1, int p2, int p3) {
+  if (p1 <= p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case11(int p1, int p2, int p3) {
+  if (p1 < p2 <= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case12(int p1, int p2, int p3) {
+  if (p1 <= p2 <= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case13(int p1, int p2, int p3) {
+  if (p1 > p2 < p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case14(int p1, int p2, int p3) {
+  if (p1 > p2 > p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case15(int p1, int p2, int p3) {
+  if (p1 >= p2 > p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case16(int p1, int p2, int p3) {
+  if (p1 >= p2 >= p3)
+return 1;
+  return 0;
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case17(int p1, int p2, int p3) {
+  // should not warn
+  if (p1 >= p2 || p3)
+return 1;
+  return 0;
+}
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -83,6 +83,7 @@
 CHECK-NEXT:-Wextern-c-compat
 CHECK-NEXT:-Wuser-defined-warnings
 CHECK-NEXT:  -Wparentheses
+CHECK-NEXT:-Wcompare-op-parentheses
 CHECK-NEXT:-Wlogical-op-pare

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

vabridgers wrote:
> 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!
Hi @lebedev.ri , the warning emits a note that says "place parentheses around 
the '' expression to silence this warning" (see the test cases below). Is 
this sufficient, or are you looking for something else?


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

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 282484.
vabridgers added a comment.

refactor test cases per comment from Arthur


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,179 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int tests(int p1, int p2, int p3, int p4, int p5, int p6) {
+  int b = 0;
+  b = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 < p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 <= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+  b = p1 <= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+  
+  b = p1 > p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 > p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 >= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 >= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  
+  b = p1 > p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 < p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  
+  b = p1 > p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 < p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  
+  b = p1 >= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 <= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+  
+  b = p1 >= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 <= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+ 
+  b = p1 < p2 < p3 < p4;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+  b = (p1 < p2) < p3 < p4;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 < (p2 < p3) < p4;

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I believe I've addressed all comments so far. Looks like Arthur suggested some 
particular cases that are not currently covered, and are not covered by this 
change since I think addressing those issues are our of scope of my original 
intent. If this patch is otherwise acceptable, would the reviewers be ok 
accepting this patch on the condition of creating a bugzilla report to track 
those issues?


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

[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/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

vabridgers wrote:
> vabridgers wrote:
> > 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!
> Hi @lebedev.ri , the warning emits a note that says "place parentheses around 
> the '' expression to silence this warning" (see the test cases below). Is 
> this sufficient, or are you looking for something else?
https://godbolt.org/z/d1dG1o
For the very similar situation `(x & 1 == 0)`, Clang suggests //two different 
fixits//, and I believe Roman was suggesting that you should do the same kind 
of thing here.
```
:3:16: warning: & has lower precedence than ==; == will be evaluated 
first [-Wparentheses]
int y = (x & 1 == 0);
   ^~~~
:3:16: note: place parentheses around the '==' expression to silence 
this warning
int y = (x & 1 == 0);
   ^
 ( )
:3:16: note: place parentheses around the & expression to evaluate it 
first
int y = (x & 1 == 0);
   ^
 ()
```
For our situation here it would be something like
```
:3:16: warning: chained comparisons like 'x<=y<=z' are interpreted as 
'(x<=y ? 1 : 0) <= z' [-Wcompare-op-parentheses]
int w = (x < y < z);
   ^~~~
:3:16: note: place parentheses around the first '<' expression to 
silence this warning
int w = (x < y < z);
 ^
 ()
:3:16: note: separate the expression into two clauses to give it the 
mathematical meaning
int y = (x < y < z);
   ^
 () && (y)
```
Watch out that you don't suggest a wrong fixit for `(0 <= sideeffect() < 10)`, 
though. I seem to recall another warning that recently had a lot of trouble 
phrasing its fixit in a way that wouldn't invoke UB or change the behavior of 
the code in corner cases, but I forget the details.


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

[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/lib/Sema/SemaExpr.cpp:14047
 
+static bool isComparisonOpSamePrecedence(BinaryOperatorKind Opc) {
+  switch (Opc) {

Same precedence as what?
I think this should just be called `isRelationalOperator`, and I would bet 
money that such a classifier function already exists somewhere in the codebase.


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

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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:14047
 
+static bool isComparisonOpSamePrecedence(BinaryOperatorKind Opc) {
+  switch (Opc) {

Quuxplusone wrote:
> Same precedence as what?
> I think this should just be called `isRelationalOperator`, and I would bet 
> money that such a classifier function already exists somewhere in the 
> codebase.
`isRelationalOp` and its a member function of `BinaryOperator` so there is no 
need to even access the OperatorKind.



Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

Quuxplusone wrote:
> vabridgers wrote:
> > vabridgers wrote:
> > > 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!
> > Hi @lebedev.ri , the warning emits a note that says "place parentheses 
> > around the '' expression to silence this warning" (see the test cases 
> > below). Is this sufficient, or are you looking for something else?
> https://godbolt.org/z/d1dG1o
> For the very similar situation `(x & 1 == 0)`, Clang suggests //two different 
> fixits//, and I believe Roman was suggesting that you should do the same kind 
> of thing here.
> ```
> :3:16: warning: & has lower precedence than ==; == will be evaluated 
> first [-Wparentheses]
> int y = (x & 1 == 0);
>^~~~
> :3:16: note: place parentheses around the '==' expression to silence 
> this warning
> int y = (x & 1 == 0);
>^
>  ( )
> :3:16: note: place parentheses around the & expression to evaluate it 
> first
> int y = (x & 1 == 0);
>^
>  ()
> ```
> For our situation here it would be something like
> ```
> :3:16: warning: chained comparisons like 'x<=y<=z' are interpreted as 
> '(x<=y ? 1 : 0) <= z' [-Wcompare-op-parentheses]
> int w = (x < y < z);
>^~~~
> :3:16: note: place parentheses around the first '<' expression to 
> silence this warning
> int w = (x < y < z);
>  ^
>  ()
> :3:16: note: separate the expression into two clauses to give it the 
> mathematical meaning
> int y = (x < y < z);
>^
>  () && (y)
> ```
> Watch out that you don't suggest a wrong fixit for `(0 <= sideeffect() < 
> 10)`, though. I seem to recall another warning that recently had a lot of 
> trouble phrasing its fixit in a way that wouldn't invoke UB or change the 
> behavior of the code in corner cases, but I forget the details.
Surely you'd just need to check `y` for side effects before creating the fix-it.


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

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 282852.
vabridgers added a comment.

isRelationalOp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,176 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int tests(int p1, int p2, int p3, int p4, int p5, int p6) {
+  int b = 0;
+  b = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 < p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 <= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+  b = p1 <= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+
+  b = p1 > p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 > p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 >= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 >= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+
+  b = p1 > p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+
+  b = p1 > p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 < p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+
+  b = p1 >= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 <= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+
+  b = p1 >= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 <= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+
+  b = p1 < p2 < p3 < p4;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+  b = (p1 < p2) < p3 < p4;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 < (p2 < p3) < p4;
+  // expected-warning@-1 {{comparisons like '

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 282854.
vabridgers added a comment.

fix misc test formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,176 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int tests(int p1, int p2, int p3, int p4, int p5, int p6) {
+  int b = 0;
+  b = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 < p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 <= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+  b = p1 <= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+
+  b = p1 > p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 > p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 >= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 >= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+
+  b = p1 > p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+
+  b = p1 > p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  b = p1 < p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+
+  b = p1 >= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 <= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+
+  b = p1 >= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' expression to silence this warning}}
+  b = p1 <= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' expression to silence this warning}}
+
+  b = p1 < p2 < p3 < p4;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+  b = (p1 < p2) < p3 < p4;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  b = p1 < (p2 < p3) < p4;
+  // expected-warning@-1 {{comparis

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked 2 inline comments as done.
vabridgers added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:14047
 
+static bool isComparisonOpSamePrecedence(BinaryOperatorKind Opc) {
+  switch (Opc) {

njames93 wrote:
> Quuxplusone wrote:
> > Same precedence as what?
> > I think this should just be called `isRelationalOperator`, and I would bet 
> > money that such a classifier function already exists somewhere in the 
> > codebase.
> `isRelationalOp` and its a member function of `BinaryOperator` so there is no 
> need to even access the OperatorKind.
Fixed, thanks



Comment at: clang/lib/Sema/SemaExpr.cpp:14047
 
+static bool isComparisonOpSamePrecedence(BinaryOperatorKind Opc) {
+  switch (Opc) {

vabridgers wrote:
> njames93 wrote:
> > Quuxplusone wrote:
> > > Same precedence as what?
> > > I think this should just be called `isRelationalOperator`, and I would 
> > > bet money that such a classifier function already exists somewhere in the 
> > > codebase.
> > `isRelationalOp` and its a member function of `BinaryOperator` so there is 
> > no need to even access the OperatorKind.
> Fixed, thanks
Fixed, thanks



Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

njames93 wrote:
> Quuxplusone wrote:
> > vabridgers wrote:
> > > vabridgers wrote:
> > > > 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!
> > > Hi @lebedev.ri , the warning emits a note that says "place parentheses 
> > > around the '' expression to silence this warning" (see the test cases 
> > > below). Is this sufficient, or are you looking for something else?
> > https://godbolt.org/z/d1dG1o
> > For the very similar situation `(x & 1 == 0)`, Clang suggests //two 
> > different fixits//, and I believe Roman was suggesting that you should do 
> > the same kind of thing here.
> > ```
> > :3:16: warning: & has lower precedence than ==; == will be 
> > evaluated first [-Wparentheses]
> > int y = (x & 1 == 0);
> >^~~~
> > :3:16: note: place parentheses around the '==' expression to 
> > silence this warning
> > int y = (x & 1 == 0);
> >^
> >  ( )
> > :3:16: note: place parentheses around the & expression to evaluate 
> > it first
> > int y = (x & 1 == 0);
> >^
> >  ()
> > ```
> > For our situation here it would be something like
> > ```
> > :3:16: warning: chained comparisons like 'x<=y<=z' are interpreted 
> > as '(x<=y ? 1 : 0) <= z' [-Wcompare-op-parentheses]
> > int w = (x < y < z);
> >^~~~
> > :3:16: note: place parentheses around the first '<' expression to 
> > silence this warning
> > int w = (x < y < z);
> >  ^
> >  ()
> > :3:16: note: separate the expression into two clauses to give it 
> > the mathematical meaning
> > int y = (x < y < z);
> >^
> >  () && (y)
> > ```
> > Watch out that you don't suggest a wrong fixit for `(0 <= sideeffect() < 
> > 10)`, though. I seem to recall another warning that recently had a lot of 
> > trouble phrasing its fixit in a way that wouldn't invoke UB or change the 
> > behavior of the code in corner cases, but I forget the details.
> Surely you'd just need to check `y` for side effects before creating the 
> fix-it.
Still working on these, thanks



Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

vabridgers wrote:
> njames93 wrote:
> > Quuxplusone wrote:
> > > vabridgers wrote:
> > > > vabridgers wrote:
> > > > > 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!
> > > > Hi @lebedev.ri , the warning emits a note that says "place parentheses 
> > > > around the '' expression to silence this warning" (see the test 
> > > > cases below). Is this sufficient, or are you looking for something else?
> > > https://godbolt.org/z/d1dG1o
> > > For the very similar situation `(x & 1 == 0)`, Clang suggests //two 
> > > different fixits//, and I believe Roman was suggesting that you should do 
> > > the same kind of thing here.
> > > ```
> > > :3:16: warning: & has lower precedence than ==; == will be 
> > > evaluated first [-Wparentheses]
> > > int y = (x & 1 == 0);
> > >^~~~
> > > :3:16: note: place parentheses around the '==' expressi

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 283447.
vabridgers added a comment.

added prototype fixits for review.
added additional RUN test case.
filed https://bugs.llvm.org/show_bug.cgi?id=47010 for other 
warnings improvement post landing of this patch, after lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,245 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wparentheses -verify %s
+
+int tests(int p1, int p2, int p3, int p4, int p5, int p6) {
+  int b = 0;
+  b = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 < p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 <= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 <= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+
+  b = p1 > p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 > p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 >= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 >= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+
+  b = p1 > p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-4 {{seperate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I added prototype fixits per request by Roman, updated the LIT test, and added 
an additional RUN line (one for -Wparentheses and one for 
-Wcompare-op-parentheses). Also filed 
https://bugs.llvm.org/show_bug.cgi?id=47010 to follow up on the FIXME cases at 
the bottom of the LIT since they are out of scope for this change. Thanks for 
the feedback and comments so far, I look forward to driving this change to 
completion.


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

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Also with a name like compare op parenthesis. It sounds like this would 
consider `==` and `!=`




Comment at: clang/lib/Sema/SemaExpr.cpp:14034
+  << FixItHint::CreateInsertion(LHSExpr->getEndLoc(), ") && ")
+  << FixItHint::CreateInsertion(LHSBO->getRHS()->getBeginLoc(), "(y ")
+  << FixItHint::CreateInsertion(RHSExpr->getEndLoc(), ")");

You don't want to insert `y` but the source code for `y`


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

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:14034
+  << FixItHint::CreateInsertion(LHSExpr->getEndLoc(), ") && ")
+  << FixItHint::CreateInsertion(LHSBO->getRHS()->getBeginLoc(), "(y ")
+  << FixItHint::CreateInsertion(RHSExpr->getEndLoc(), ")");

njames93 wrote:
> You don't want to insert `y` but the source code for `y`
Gotcha, I'll address.


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

[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/include/clang/Basic/DiagnosticSemaKinds.td:6139
+def note_compare_seperate_sides : Note<
+  "seperate the expression into two clauses to give it the intended 
mathematical meaning">;
+

Both in the string and in the identifier, s/seperate/separate/.
I would also s/sides/clauses/ or s/sides/expression/ just to avoid giving too 
many different names to the same entity.



Comment at: clang/lib/Sema/SemaExpr.cpp:14034
+  << FixItHint::CreateInsertion(LHSExpr->getEndLoc(), ") && ")
+  << FixItHint::CreateInsertion(LHSBO->getRHS()->getBeginLoc(), "(y ")
+  << FixItHint::CreateInsertion(RHSExpr->getEndLoc(), ")");

vabridgers wrote:
> njames93 wrote:
> > You don't want to insert `y` but the source code for `y`
> Gotcha, I'll address.
Please make sure to add at least one regression test to catch this (i.e. some 
test that tests the wording of the fixit and uses a name for the middle term 
that is not `y`). I don't know how to test a fixit, but there must be a way.



Comment at: clang/lib/Sema/SemaExpr.cpp:14211
+
+  // Warn on ambiguous compares, loosely x<=y<=z
+  if (BinaryOperator::isRelationalOp(Opc))

FWIW, I would s/loosely/such as/.



Comment at: clang/lib/Sema/SemaExpr.cpp:14010
+  << Bop->getSourceRange() << OpLoc;
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)

vabridgers wrote:
> vabridgers wrote:
> > njames93 wrote:
> > > Quuxplusone wrote:
> > > > vabridgers wrote:
> > > > > vabridgers wrote:
> > > > > > 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!
> > > > > Hi @lebedev.ri , the warning emits a note that says "place 
> > > > > parentheses around the '' expression to silence this warning" 
> > > > > (see the test cases below). Is this sufficient, or are you looking 
> > > > > for something else?
> > > > https://godbolt.org/z/d1dG1o
> > > > For the very similar situation `(x & 1 == 0)`, Clang suggests //two 
> > > > different fixits//, and I believe Roman was suggesting that you should 
> > > > do the same kind of thing here.
> > > > ```
> > > > :3:16: warning: & has lower precedence than ==; == will be 
> > > > evaluated first [-Wparentheses]
> > > > int y = (x & 1 == 0);
> > > >^~~~
> > > > :3:16: note: place parentheses around the '==' expression to 
> > > > silence this warning
> > > > int y = (x & 1 == 0);
> > > >^
> > > >  ( )
> > > > :3:16: note: place parentheses around the & expression to 
> > > > evaluate it first
> > > > int y = (x & 1 == 0);
> > > >^
> > > >  ()
> > > > ```
> > > > For our situation here it would be something like
> > > > ```
> > > > :3:16: warning: chained comparisons like 'x<=y<=z' are 
> > > > interpreted as '(x<=y ? 1 : 0) <= z' [-Wcompare-op-parentheses]
> > > > int w = (x < y < z);
> > > >^~~~
> > > > :3:16: note: place parentheses around the first '<' expression 
> > > > to silence this warning
> > > > int w = (x < y < z);
> > > >  ^
> > > >  ()
> > > > :3:16: note: separate the expression into two clauses to give 
> > > > it the mathematical meaning
> > > > int y = (x < y < z);
> > > >^
> > > >  () && (y)
> > > > ```
> > > > Watch out that you don't suggest a wrong fixit for `(0 <= sideeffect() 
> > > > < 10)`, though. I seem to recall another warning that recently had a 
> > > > lot of trouble phrasing its fixit in a way that wouldn't invoke UB or 
> > > > change the behavior of the code in corner cases, but I forget the 
> > > > details.
> > > Surely you'd just need to check `y` for side effects before creating the 
> > > fix-it.
> > Still working on these, thanks
> still working on these, thanks
My understanding is that option (2) `(x <= (y <= z))` is never suggested. It's 
reasonable not to suggest it, because it is neither "what the compiler sees, 
elucidated for the human reader" nor "what the programmer meant, corrected for 
the compiler." However, as it's not suggested, it shouldn't be documented in 
this comment.



Comment at: clang/test/Misc/warning-wall.c:86
 CHECK-NEXT:  -Wparentheses
+CHECK-NEXT:-Wcompare-op-parentheses
 CHECK-NEXT:-Wlogical-op-parentheses

My impression is that you (Vincent) are tending toward just adding `x == y == 
z` and `x <= y == z` to this same `-Wcompare-op-parentheses`. That's also my 
preference (except that I wish it could be done all together in one big pull 
request instead of being split up and perhaps deferred).  Have we conclusively 
established t

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

The diagnostic we get for the case of three or more comparison operators here 
doesn't seem ideal. Perhaps we could do this check as part of the 
`SemaChecking` pass over the completed expression rather than doing it as we 
form the individual comparisons? That way we'll have the contextual information 
necessary to find the complete chained comparison; we'd also be able to detect 
the special case where the operator sequence is the operand of an `&`/`|`/`^` 
so that we need parentheses in the fixit.




Comment at: clang/docs/DiagnosticsReference.rst:1-5
 ..
   ---
   NOTE: This file is automatically generated by running clang-tblgen
   -gen-diag-docs. Do not edit this file by hand!!
   ---

Please note this :)

You can revert the changes to this file; it's auto-generated. (We only check in 
a version because the clang website infrastructure isn't yet able to generate 
it on-demand.)



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6137
+def note_compare_with_parens : Note<
+  "place parentheses around the '%0' comparison to silence this warning">;
+def note_compare_seperate_sides : Note<

If you're going to quote the operator here, you should prepend "first" or 
similar if both operators are the same.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6139
+def note_compare_seperate_sides : Note<
+  "seperate the expression into two clauses to give it the intended 
mathematical meaning">;
+

Quuxplusone wrote:
> Both in the string and in the identifier, s/seperate/separate/.
> I would also s/sides/clauses/ or s/sides/expression/ just to avoid giving too 
> many different names to the same entity.
Don't say "intended". We don't know the intent.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6135
+  "comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'">,
+  InGroup, DefaultIgnore;
+

vabridgers wrote:
> Quuxplusone wrote:
> > Why is this `x<=y<=z` instead of the simpler `x > "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 ...
> 
> -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.
> ...
> 
> 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!
> 
Per our diagnostic best practices 
(http://clang.llvm.org/docs/InternalsManual.html#the-format-string), don't 
repeat the code in the diagnostic (or even an analogy of it such as 'x<=y<=z'); 
instead, consider underlining the chained comparison operators in the snippet. 
Also keep in mind that the developer will see the diagnostic and the notes 
together; you can use the notes to help clarify the meaning of the diagnostic:

```
error: chained comparison does not have its mathematical meaning
  if (a <= b < c)
 ^~   ~
note: perform two separate comparisons to compare the same operand twice
  if (a <= b < c)
 ^
 && b
note: place parentheses around the first comparison to compare against its 
'bool' result and silence this warning
  if (a <= b < c)
  ^
  ( )
```



Comment at: clang/lib/Sema/SemaExpr.cpp:14008-14010
+// Accepts (x ['<'|'<='|'>'|'>='] y ['<'|'<='|'>'|'>='] z), suggests:
+// 1) ((x ['<'|'<='|'>'|'>='] y) ['<'|'<='|'>'|'>='] z), or
+// 2) (x ['<'|'<='|'>'|'>='] (y ['<'|'<='|'>'|'>='] z)). or

Why not also `==` and `!=` here? `a < b == c < d` is a perfectly reasonable 
mathematical equation (modulo the spelling of `==`), and it would seem sensible 
to catch that with the same diagnostic.



Comment at: clang/lib/Sema/SemaExpr.cpp:14031
+
+  Self.Diag(LHSExpr->getBeginLoc(), diag::note_compare_seperate_sides)
+  << FixItHint::CreateInsertion(LHSExpr->getBeginLoc(), "(")

I think we should consider omitting the fixit if the `y` expression is long or 
has side-effects.



Comment at: clang/lib/Sema/SemaExpr.cpp:14032-14035
+  << 

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

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 283780.
vabridgers added a comment.

use source from expression in fixit
s/seperate/separate/
address some chained comparison ambiguities outside of original scope of changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097

Files:
  clang/docs/DiagnosticsReference.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-compare-op-parentheses.c

Index: clang/test/Sema/warn-compare-op-parentheses.c
===
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,263 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wparentheses -verify %s
+
+int tests(int p1, int p2, int p3, int p4, int p5, int p6) {
+  int b = 0;
+  b = p1 < p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 < p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 <= p2 <= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 <= p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '<=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+
+  b = p1 > p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 > p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 >= p2 >= p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 >= p2 > p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>=' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+
+  b = p1 > p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence this warning}}
+  // expected-note@-3 {{place parentheses around the '<' comparison to silence this warning}}
+  // expected-note@-4 {{separate the expression into two clauses to give it the intended mathematical meaning}}
+  b = p1 p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses aroun

[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 recent comments. I just pushed a few improvements over the last 
patch that didn't comprehend latest comments from @rsmith and @Quuxplusone. 
I'll read through those carefully and address those.


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D85097#2201609 , @vabridgers wrote:

> Thanks for the recent comments. I just pushed a few improvements over the 
> last patch that didn't comprehend latest comments from @rsmith and 
> @Quuxplusone. I'll read through those carefully and address those.

Reverse-ping, thanks. I think this is a worthwhile 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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/test/Sema/warn-compare-op-parentheses.c:49
+  b = p1 > p2 < p3;
+  // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as 
'(x<=y ? 1 : 0) <= z'}}
+  // expected-note@-2 {{place parentheses around the '>' comparison to silence 
this warning}}

Not very user friendly warning text..


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

This review seems to be stuck/dead, consider abandoning if no longer relevant.


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