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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097