[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 '<op>' 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.
> > > > ```
> > > > <source>:3:16: warning: & has lower precedence than ==; == will be evaluated first [-Wparentheses]
> > > >     int y = (x & 1 == 0);
> > > >                ^~~~~~~~
> > > > <source>:3:16: note: place parentheses around the '==' expression to silence this warning
> > > >     int y = (x & 1 == 0);
> > > >                ^
> > > >                  (     )
> > > > <source>: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
> > > > ```
> > > > <source>: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);
> > > >                ^~~~~~~~
> > > > <source>:3:16: note: place parentheses around the first '<' expression to silence this warning
> > > >     int w = (x < y < z);
> > > >              ^
> > > >              (    )
> > > > <source>: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 that everyone's on board for this?

An alternative (but IMHO a poor alternative) would be to have two or three more granular warnings, e.g. `-Wrelational-op-parentheses` and `-Wequality-op-parentheses` (and `-Wmixed-relational-equality-op-parentheses`?!). If we were to foresee that happening, then `-Wcompare-op-parentheses` would be the wrong name for this warning option today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097