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


================
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 '<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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85097