aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6654 +def warn_comp_op_in_comp_op : Warning< + "'%0' within '%1'">, InGroup<ComparisonOpParentheses>, DefaultIgnore; +def warn_rel_op_in_rel_op : Warning< ---------------- Spit-balling wording ideas: `'%0' results in a boolean value which is then compared using '%1'; did you mean a three-way comparison instead?` `comparing the boolean result of '%0' with '%1' does not perform a three-way comparison` Also, why are these diagnostics off by default? Do we have some idea as to the false positive rate? ================ Comment at: clang/lib/Sema/SemaExpr.cpp:15548 + + SuggestParentheses(Self, Bop->getOperatorLoc(), + Self.PDiag(diag::note_precedence_silence) ---------------- erichkeane wrote: > hazohelet wrote: > > erichkeane wrote: > > > I find myself wondering if we could provide a better 'suggested fix' > > > here. Aaron is better with the diagnostics than I am, but I would think > > > that someone doing: > > > > > > `a < b < c` PROBABLY doesn't mean that, they probably mean: `a < b && b < > > > c`. > > > > > > Also, is the mistake the 'same' when they do something like `a > b != c` > > > ? It would seem to me that mixing operators might make it something > > > either more intentional/meaningful. ADDITIONALLY, in the case where they > > > are booleans, these end up being overly noisy. The pattern of the == c > > > (where 'c' is bool, or convertible to bool) is probably intentional. > > > > > > I think the logic here needs to be more complicated than just "Comparison > > > within Comparison", however I don't have a fully formed idea of when to > > > diagnose. > > > > > > @tahonermann : Do you perhaps have a good idea? > > > I find myself wondering if we could provide a better 'suggested fix' > > > here. Aaron is better with the diagnostics than I am, but I would think > > > that someone doing: > > > > > > `a < b < c` PROBABLY doesn't mean that, they probably mean: `a < b && b < > > > c`. > > > > Yes. We could provide a better fix-it hint. > > My idea: > > - In the case of chained relational operators (`<`, `>`, `<=`, `>=`), clang > > suggests adding `&&`. > > - In other cases, clang suggests parentheses. > > How about doing it this way? It's similar to how Rust handles chained > > comparisons. > > > > > Also, is the mistake the 'same' when they do something like `a > b != c` > > > ? It would seem to me that mixing operators might make it something > > > either more intentional/meaningful. ADDITIONALLY, in the case where they > > > are booleans, these end up being overly noisy. The pattern of the == c > > > (where 'c' is bool, or convertible to bool) is probably intentional. > > > > > > I think the logic here needs to be more complicated than just "Comparison > > > within Comparison", however I don't have a fully formed idea of when to > > > diagnose. > > > > I have a differing perspective on suppressing the warning for boolean and > > boolean-convertible values. > > There are two possible programmer mistakes in chained comparisons. > > 1. `a > b != c` misunderstood as `a > b && b != c` > > 2. `a > b != c` misunderstood as `a > (b != c)` > > While the latter is considered rare in this scenario, the former could be > > likely to happen due to other languages, such as Python handling chained > > comparisons in the former manner. > > > > > I'd be interested to see the fixit-hints for the first bit, also to see how > others feel about it here. > > > IMO, `a > b != c` to mean `(a > b) != c` is a reasonably common pattern I > suspect we won't want to be noisy on. Common? I did a code search and everything I'm seeing is either a false positive (a lot of template stuff) or within a comment: https://sourcegraph.com/search?q=context:global+%5BA-Za-z0-9_%5D%2B%5Cs*%28%5C%3D%5C%3D%7C%5C%21%5C%3D%7C%5C%3C%7C%5C%3E%7C%5C%3C%5C%3D%7C%5C%3E%5C%3D%29%5Cs*%5BA-Za-z0-9_%5D%2B%5Cs*%5B%5C%21%5C%3D%5D%5C%3D%5Cs*%5BA-Za-z0-9_%5D%2B+lang:c+lang:C%2B%2B+&patternType=regexp&sm=1&groupBy=repo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142800/new/ https://reviews.llvm.org/D142800 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
