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

[Aaron Ballman via Phabricator via cfe-commits]

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