erichkeane added inline comments.
================ Comment at: clang/lib/Sema/SemaExpr.cpp:15548 + + SuggestParentheses(Self, Bop->getOperatorLoc(), + Self.PDiag(diag::note_precedence_silence) ---------------- 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. 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
