vabridgers added a comment. Thanks for the comments. I posted an update for the simpler issues, working on refactoring the test cases and creating a fixit. Thanks for the good and actionable review comments!
================ Comment at: clang/docs/DiagnosticsReference.rst:2853 +-Wcompare-no-parentheses +-------------------------------- ---------------- Quuxplusone wrote: > s/-no-/-op-/ fixed, thank you! ================ Comment at: clang/docs/DiagnosticsReference.rst:9885 -Also controls `-Wbitwise-conditional-parentheses`_, `-Wbitwise-op-parentheses`_, `-Wdangling-else`_, `-Wlogical-not-parentheses`_, `-Wlogical-op-parentheses`_, `-Woverloaded-shift-op-parentheses`_, `-Wparentheses-equality`_, `-Wshift-op-parentheses`_. +Also controls `-Wbitwise-conditional-parentheses`_, `-Wbitwise-op-parentheses`_, `-Wcompare-no-parentheses`, `-Wdangling-else`_, `-Wlogical-not-parentheses`_, `-Wlogical-op-parentheses`_, `-Woverloaded-shift-op-parentheses`_, `-Wparentheses-equality`_, `-Wshift-op-parentheses`_. ---------------- Quuxplusone wrote: > s/-no-/-op-/ > And what's going on with all these trailing underscores? If they're > important, you're missing one. missed that, thank you! ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6135 + "comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'">, + InGroup<CompareOpParentheses>, DefaultIgnore; + ---------------- Quuxplusone wrote: > Why is this `x<=y<=z` instead of the simpler `x<y<z` or even `x<=y<z` (the > "half-open range" common case)? > IMHO you should mention the name "chained comparisons" here, since I think > that's what people coming from such languages will understand. Thanks Arthur. I modeled the warning message after gcc's warning message. We found internally that while gcc detected this, clang did not. See https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options ... <q> -Wparentheses Warn if parentheses are omitted in certain contexts, such as when there is an assignment in a context where a truth value is expected, or when operators are nested whose precedence people often get confused about. Also warn if a comparison like x<=y<=z appears; this is equivalent to (x<=y ? 1 : 0) <= z, which is a different interpretation from that of ordinary mathematical notation. ... </q> While this is the gcc documentation, we can craft whatever message we see fit at this point in time :) I'll add "chained" comparison for this next update, we can tailor the message as we see fit. Thanks! ================ Comment at: clang/lib/Sema/SemaExpr.cpp:14010 + << Bop->getSourceRange() << OpLoc; + SuggestParentheses(Self, Bop->getOperatorLoc(), + Self.PDiag(diag::note_precedence_silence) ---------------- 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! ================ Comment at: clang/test/Sema/warn-compare-op-parentheses.c:1 +// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s + ---------------- Quuxplusone wrote: > These are some great test cases, but I think they would benefit from being > exhaustive. > > ``` > int tests(int p1, int p2, int p3) { > bool b; > b = (p1 < p2 < p3); // expected-warning{{comparisons like 'x<y<z' are > interpreted}} expected-note{{place parentheses}} > b = (p1 < p2 <= p3); // expected-warning{{comparisons like 'x<y<z' are > interpreted}} expected-note{{place parentheses}} > b = (p1 < p2 > p3); // expected-warning{{comparisons like 'x<y<z' are > interpreted}} expected-note{{place parentheses}} > b = (p1 < p2 >= p3); // expected-warning{{comparisons like 'x<y<z' are > interpreted}} expected-note{{place parentheses}} > b = (p1 <= p2 < p3); // expected-warning{{comparisons like 'x<y<z' are > interpreted}} expected-note{{place parentheses}} > b = (p1 <= p2 <= p3); // expected-warning{{comparisons like 'x<y<z' are > interpreted}} expected-note{{place parentheses}} > ``` > > etc. etc. > I don't see any downside to being systematic here. I'll address in a future update. Thank you! ================ Comment at: clang/test/Sema/warn-compare-op-parentheses.c:129 + return 0; +} ---------------- Quuxplusone wrote: > I would like to see explicit (and preferably exhaustive or at least > systematic) test cases for the "no warning intended" case: > > if ((p1 < p2) < p3) > if (p1 < (p2 < p3)) > if (0 <= (p1 < p2)) // this should already trigger a > constant-comparison-result warning, yes? > > I would like to see explicit (and preferably exhaustive or at least > systematic) test cases for mixed relational and equality comparisons: > > if (p1 == p2 < p3) // maybe intentional, but definitely deserving of a > warning > if (p1 == p2 == p3) // definitely buggy and deserving of a warning > if (p1 != p2 != p3) // definitely buggy and deserving of a warning > if (p1 < p2 == p3 < p4) // maybe intentional, but definitely deserving > of a warning > if (p1 == p2 < p3 == p4) // definitely buggy and deserving of a warning Great suggestions, I'll address in a future update. Thank you! 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
