[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

[Erich Keane via Phabricator via cfe-commits]

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