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