[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons
Erich Keane via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 31 07:04:14 PST 2023
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
More information about the cfe-commits
mailing list