rsmith added a comment. The diagnostic we get for the case of three or more comparison operators here doesn't seem ideal. Perhaps we could do this check as part of the `SemaChecking` pass over the completed expression rather than doing it as we form the individual comparisons? That way we'll have the contextual information necessary to find the complete chained comparison; we'd also be able to detect the special case where the operator sequence is the operand of an `&`/`|`/`^` so that we need parentheses in the fixit.
================ Comment at: clang/docs/DiagnosticsReference.rst:1-5 .. ------------------------------------------------------------------- NOTE: This file is automatically generated by running clang-tblgen -gen-diag-docs. Do not edit this file by hand!! ------------------------------------------------------------------- ---------------- Please note this :) You can revert the changes to this file; it's auto-generated. (We only check in a version because the clang website infrastructure isn't yet able to generate it on-demand.) ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6137 +def note_compare_with_parens : Note< + "place parentheses around the '%0' comparison to silence this warning">; +def note_compare_seperate_sides : Note< ---------------- If you're going to quote the operator here, you should prepend "first" or similar if both operators are the same. ================ 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">; + ---------------- Quuxplusone wrote: > 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. Don't say "intended". We don't know the intent. ================ 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; + ---------------- vabridgers wrote: > 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! > Per our diagnostic best practices (http://clang.llvm.org/docs/InternalsManual.html#the-format-string), don't repeat the code in the diagnostic (or even an analogy of it such as 'x<=y<=z'); instead, consider underlining the chained comparison operators in the snippet. Also keep in mind that the developer will see the diagnostic and the notes together; you can use the notes to help clarify the meaning of the diagnostic: ``` error: chained comparison does not have its mathematical meaning if (a <= b < c) ^~ ~ note: perform two separate comparisons to compare the same operand twice if (a <= b < c) ^ && b note: place parentheses around the first comparison to compare against its 'bool' result and silence this warning if (a <= b < c) ^ ( ) ``` ================ Comment at: clang/lib/Sema/SemaExpr.cpp:14008-14010 +// Accepts (x ['<'|'<='|'>'|'>='] y ['<'|'<='|'>'|'>='] z), suggests: +// 1) ((x ['<'|'<='|'>'|'>='] y) ['<'|'<='|'>'|'>='] z), or +// 2) (x ['<'|'<='|'>'|'>='] (y ['<'|'<='|'>'|'>='] z)). or ---------------- Why not also `==` and `!=` here? `a < b == c < d` is a perfectly reasonable mathematical equation (modulo the spelling of `==`), and it would seem sensible to catch that with the same diagnostic. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:14031 + + Self.Diag(LHSExpr->getBeginLoc(), diag::note_compare_seperate_sides) + << FixItHint::CreateInsertion(LHSExpr->getBeginLoc(), "(") ---------------- I think we should consider omitting the fixit if the `y` expression is long or has side-effects. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:14032-14035 + << FixItHint::CreateInsertion(LHSExpr->getBeginLoc(), "(") + << FixItHint::CreateInsertion(LHSExpr->getEndLoc(), ") && ") + << FixItHint::CreateInsertion(LHSBO->getRHS()->getBeginLoc(), "(y ") + << FixItHint::CreateInsertion(RHSExpr->getEndLoc(), ")"); ---------------- The parentheses here clutter and distract from the point of the fixit (adding the `&& y`). We usually don't need them, and in the cases where we do need them, the parens you added here aren't correct. For example, `cond1 && a + b < c < d + e && cond2` doesn't need any parens, and `a | b < c < d` needs parens around the entire rewritten `b < c && c < d` expression, not around the individual terms. If you want the rewrite to always be correct, you should add a `(` at the start, an `)` at the end, and a `&& y` before the second operator (or a `y &&` after the first operator). If you're OK with it being wrong in the weird case of an `&` / `|` / `^` operator adjacent to the chained comparison, then no parens are needed at all. But there's no need to parenthesize the individual operands of the `&&`. 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
