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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: tahonermann.
erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:15539
+/// Emit a diagnostic together with a fixit hint that wraps the inner comparison
+/// expression in parentheses.
+static void EmitDiagnosticForCompOpInCompOp(Sema &Self, SourceLocation OpLoc,
----------------
I'd repalce the comment with something more like:

```Emit a diagnostic for a case where a comparison operation is a direct sub-expression of another comparison operation.  Additionally, emit a fixit hint to suggest the inner comparison expression be wrapped in parentheses.```


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


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

https://reviews.llvm.org/D142800