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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
