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

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added inline comments.


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




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142800/new/

https://reviews.llvm.org/D142800