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

[hazohelet]

Summary

This patch introduces a new warning flag -Wcomparison-op-parentheses in -Wparentheses to issue warnings with its fixit hint for comparison operators within another comparison operator.

This fixes https://github.com/llvm/llvm-project/issues/60256

[hazohelet]

Fix the new warning issued against libcxx test code.

[erichkeane]

clang/lib/Sema/SemaExpr.cpp

15577

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.

15586

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?

clang/test/Sema/comparison-op-parentheses.c

2

don't use macros to control conditional diagnostics like that. You use -verify=something.

[hazohelet]

clang/lib/Sema/SemaExpr.cpp

15586

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.

[erichkeane]

clang/lib/Sema/SemaExpr.cpp

15586

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.

[dblaikie]

I'd be interested to see the fixit-hints for the first bit, also to see how others feel about it here.

My 2c is that -Wparentheses is already a very stylistic warning (even correct code, without knowing about this esoteric/specific suppression style of adding parens, is likely to be caught by this general class of diagnostic) so I'm pretty apprehensive about opening it up further - but data would be the decider. Running the proposed slices (different operators in different orders, etc) of warning over some broad codebases to get a sense of the false positive rate would be really helpful/necessary to decide which things to include and exclude.

[hazohelet]

Address comments from erichkeane:

• Fix comment wording
• Avoid using macro in test file

Implement my proposal for fix-it hint:

• In the case of chained relational operators (<, >, <=, >=), suggest adding &&.
• In other cases, suggest parentheses.

[erichkeane]

I'm reasonably happy here. I think the chained-comparisons getting the && suggestion is nice. However in each case, the diagnostic is REALLY poor. It isn't really clear to most folks (barely clear to me!) that '<' within '>' really means anything. I'd love if we could spend some time brainstorming some better diagnostic wording that makes it clear what the problem is.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6654

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?

clang/lib/Sema/SemaExpr.cpp

15586

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

[hazohelet]

Also, why are these diagnostics off by default? Do we have some idea as to the false positive rate?

As for the false positive rate, I have checked for instances of this warning in the codebases for 'oneapi-src/oneTBB', 'rui314/mold', and 'microsoft/lightgbm', but did not find any new cases.
I also ran a test on 'tensorflow/tensorflow' using gcc '-Wparentheses' and found six lines of code that trigger the new diagnostic. They all relate to checking whether x and y have the same sign using x > 0 == y > 0 and alike. I tried to build with tensorflow using clang, but I stumbled upon some errors (for my poor knowledge of bazel configuration), so here I am using gcc.

I set the diagnostic disabled by default for compatibility with gcc. Considering the test against tensorflow above, it would be too noisy if we turned on the suggest-parentheses diagnostic by default (From my best guess, it would generate at least 18 new instances of warning on the tensorflow build for the six lines).
However, in my opinion, it is reasonable enough to have the diagnostic on chained relational operators enabled by default. The following is an excerpt from the 'Existing Code in C++' section of the proposal document of the introduction of chaining comparison for C++17.

Overall, what we found was:

• Zero instances of chained arithmetic comparisons that are correct today. That is, intentionally using the current standard behavior.
• Four instances of currently-erroneous arithmetic chaining, of the assert(0 <= ratio <= 1.0); variety. These are bugs that compile today but don’t do what the programmer intended, but with this proposal would change in meaning to become correct.
• Many instances of using successive comparison operators in DSLs that overloaded these operators to give meaning unrelated to comparisons.

Note that the 'chaining comparisons' in the document are

• all ==, such as a == b == c == d;
• all {<, <=}, such as a < b <= c < d; and
• all {>, >=} (e.g., a >= b > c > d).

URL: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0893r0.html

Although this paper is five years old now, I think we can conclude chaining relational operators are bug-prone and should be diagnosed by default.
Also, reading the document above, I think it would be reasonable to suggest adding '&&' in a == b == c case, too.

tensorflow/tensorflow newly-warned lines:

• https://github.com/tensorflow/tensorflow/blob/12eef948f12e47ab9ce736d8967c72fc4436db79/tensorflow/compiler/xla/overflow_util.h#L67
• https://github.com/tensorflow/tensorflow/blob/78034c6ee8f20932d7c43498326ea190966c5a19/tensorflow/core/kernels/cwise_ops.h#L366
• https://github.com/tensorflow/tensorflow/blob/78034c6ee8f20932d7c43498326ea190966c5a19/tensorflow/core/kernels/cwise_ops.h#L436
• https://github.com/tensorflow/tensorflow/blob/78034c6ee8f20932d7c43498326ea190966c5a19/tensorflow/core/kernels/cwise_ops.h#L457
• https://github.com/tensorflow/tensorflow/blob/9edf0d9eb621f18217c2a0cc5d9b67f53b40901e/tensorflow/compiler/xla/service/while_loop_analysis.cc#L364
• https://github.com/tensorflow/tensorflow/blob/9edf0d9eb621f18217c2a0cc5d9b67f53b40901e/tensorflow/compiler/xla/service/while_loop_analysis.cc#L377

[aaron.ballman]

In D142800#4104241, @hazohelet wrote:

Also, why are these diagnostics off by default? Do we have some idea as to the false positive rate?

As for the false positive rate, I have checked for instances of this warning in the codebases for 'oneapi-src/oneTBB', 'rui314/mold', and 'microsoft/lightgbm', but did not find any new cases.

Thank you for doing the extra testing! Sorry for the delayed response, this review fell off my radar for a bit.

I also ran a test on 'tensorflow/tensorflow' using gcc '-Wparentheses' and found six lines of code that trigger the new diagnostic. They all relate to checking whether x and y have the same sign using x > 0 == y > 0 and alike. I tried to build with tensorflow using clang, but I stumbled upon some errors (for my poor knowledge of bazel configuration), so here I am using gcc.

Thank you for reporting this, that's really good information.

I set the diagnostic disabled by default for compatibility with gcc. Considering the test against tensorflow above, it would be too noisy if we turned on the suggest-parentheses diagnostic by default (From my best guess, it would generate at least 18 new instances of warning on the tensorflow build for the six lines).
However, in my opinion, it is reasonable enough to have the diagnostic on chained relational operators enabled by default. The following is an excerpt from the 'Existing Code in C++' section of the proposal document of the introduction of chaining comparison for C++17.

Overall, what we found was:

• Zero instances of chained arithmetic comparisons that are correct today. That is, intentionally using the current standard behavior.
• Four instances of currently-erroneous arithmetic chaining, of the assert(0 <= ratio <= 1.0); variety. These are bugs that compile today but don’t do what the programmer intended, but with this proposal would change in meaning to become correct.
• Many instances of using successive comparison operators in DSLs that overloaded these operators to give meaning unrelated to comparisons.

Note that the 'chaining comparisons' in the document are

• all ==, such as a == b == c == d;
• all {<, <=}, such as a < b <= c < d; and
• all {>, >=} (e.g., a >= b > c > d).

URL: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0893r0.html

Although this paper is five years old now, I think we can conclude chaining relational operators are bug-prone and should be diagnosed by default.
Also, reading the document above, I think it would be reasonable to suggest adding '&&' in a == b == c case, too.

I tend to agree -- I was searching around sourcegraph (https://sourcegraph.com/search?q=context:global+lang:C+lang:C%2B%2B+%5BA-Za-z0-9_%5D%2B%5Cs*%28%3D%3D%7C%21%3D%7C%3C%3D%7C%3E%3D%29%5Cs*%5BA-Za-z0-9_%5D%2B%5Cs*%28%3D%3D%7C%21%3D%7C%3C%3D%7C%3E%3D%29&patternType=regexp&sm=1&groupBy=repo) and I can't find a whole lot of evidence for chained operators outside of comments.

[hazohelet]

Update the differential

• Revise warning messages based on ideas from @aaron.ballman.
• Introduce a new warning flag -Wchaining-comparisons that is enabled by default, to warn about chaining relationals or equal operators.
• Adjust existing test files to conform with the new warning settings.

[hazohelet]

In D142800#4165090, @aaron.ballman wrote:

In D142800#4104241, @hazohelet wrote:

Also, why are these diagnostics off by default? Do we have some idea as to the false positive rate?

As for the false positive rate, I have checked for instances of this warning in the codebases for 'oneapi-src/oneTBB', 'rui314/mold', and 'microsoft/lightgbm', but did not find any new cases.

Thank you for doing the extra testing! Sorry for the delayed response, this review fell off my radar for a bit.

I also ran a test on 'tensorflow/tensorflow' using gcc '-Wparentheses' and found six lines of code that trigger the new diagnostic. They all relate to checking whether x and y have the same sign using x > 0 == y > 0 and alike. I tried to build with tensorflow using clang, but I stumbled upon some errors (for my poor knowledge of bazel configuration), so here I am using gcc.

Thank you for reporting this, that's really good information.

I set the diagnostic disabled by default for compatibility with gcc. Considering the test against tensorflow above, it would be too noisy if we turned on the suggest-parentheses diagnostic by default (From my best guess, it would generate at least 18 new instances of warning on the tensorflow build for the six lines).
However, in my opinion, it is reasonable enough to have the diagnostic on chained relational operators enabled by default. The following is an excerpt from the 'Existing Code in C++' section of the proposal document of the introduction of chaining comparison for C++17.

Overall, what we found was:

• Zero instances of chained arithmetic comparisons that are correct today. That is, intentionally using the current standard behavior.
• Four instances of currently-erroneous arithmetic chaining, of the assert(0 <= ratio <= 1.0); variety. These are bugs that compile today but don’t do what the programmer intended, but with this proposal would change in meaning to become correct.
• Many instances of using successive comparison operators in DSLs that overloaded these operators to give meaning unrelated to comparisons.

Note that the 'chaining comparisons' in the document are

• all ==, such as a == b == c == d;
• all {<, <=}, such as a < b <= c < d; and
• all {>, >=} (e.g., a >= b > c > d).

URL: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0893r0.html

Although this paper is five years old now, I think we can conclude chaining relational operators are bug-prone and should be diagnosed by default.
Also, reading the document above, I think it would be reasonable to suggest adding '&&' in a == b == c case, too.

I tend to agree -- I was searching around sourcegraph (https://sourcegraph.com/search?q=context:global+lang:C+lang:C%2B%2B+%5BA-Za-z0-9_%5D%2B%5Cs*%28%3D%3D%7C%21%3D%7C%3C%3D%7C%3E%3D%29%5Cs*%5BA-Za-z0-9_%5D%2B%5Cs*%28%3D%3D%7C%21%3D%7C%3C%3D%7C%3E%3D%29&patternType=regexp&sm=1&groupBy=repo) and I can't find a whole lot of evidence for chained operators outside of comments.

Thanks for the review and the sourcegraph search!
I added a new warning flag -Wchaining-comparisons that is enabled by default. I added this flag to -Wparentheses for gcc compatibility.

[hazohelet]

Rebase and Ping

[xgupta]

Thought to point out that there is a clang-tidy check in review doing the similar thing - https://reviews.llvm.org/D144429.

clang/docs/ReleaseNotes.rst

 - Diagnostics relating to macros on the command line of a preprocessed assembly
   file or precompiled header are now reported as coming from the file
   ``<command line>`` instead of ``<built-in>``.
 - Clang constexpr evaluator now provides a more concise diagnostic when calling
   function pointer that is known to be null.
 - Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` statements
   previously issued from ``-Wunreachable-code`` and ``-Wunreachable-code-fallthrough``
   by prioritizing ``-Wunreachable-code-fallthrough``.
+- Clang now warns by default on chaining relational operators like ``x < y > z``
+  and ``x < y <= z`` and on chaining ``==`` operators like ``x == y == z``.
+  This warning can be disabled with ``-Wno-chaining-comparisons`` 
+  Clang also warns on other successive comparison operators like ``x != y > z`` and
+  ``x == y > z`` with ``-Wcomparison-op-parentheses``. This warning is disabled by default.
+  This fixes `Issue 60256 <https://github.com/llvm/llvm-project/issues/60256>`_
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``

clang/include/clang/Basic/DiagnosticGroups.td

 def : DiagGroup<"c++2a-compat-pedantic", [CXX20CompatPedantic]>;
 
 def ExitTimeDestructors : DiagGroup<"exit-time-destructors">;
 def FlexibleArrayExtensions : DiagGroup<"flexible-array-extensions">;
 def FourByteMultiChar : DiagGroup<"four-char-constants">;
 def GlobalConstructors : DiagGroup<"global-constructors">;
 def BitwiseConditionalParentheses: DiagGroup<"bitwise-conditional-parentheses">;
 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
+def ComparisonOpParentheses: DiagGroup<"comparison-op-parentheses">;
+def ChainingComparisons: DiagGroup<"chaining-comparisons">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
 def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;
 def DanglingGsl : DiagGroup<"dangling-gsl">;

 // in -Wparentheses because most users who use -Wparentheses explicitly
 // do not want these warnings.
 def ParenthesesOnEquality : DiagGroup<"parentheses-equality">;
 def Parentheses : DiagGroup<"parentheses",
                             [LogicalOpParentheses,
                              LogicalNotParentheses,
                              BitwiseConditionalParentheses,
                              BitwiseOpParentheses,
+                             ComparisonOpParentheses,
+                             ChainingComparisons,
                              ShiftOpParentheses,
                              OverloadedShiftOpParentheses,
                              ParenthesesOnEquality,
                              DanglingElse]>;
 
 // -Wconversion has its own warnings, but we split a few out for
 // legacy reasons:
 //   - some people want just 64-to-32 warnings

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_logical_instead_of_bitwise_change_operator : Note<
   "use '%0' for a bitwise operation">;
 def note_logical_instead_of_bitwise_remove_constant : Note<
   "remove constant to silence this warning">;
 
 def warn_bitwise_op_in_bitwise_op : Warning<
   "'%0' within '%1'">, InGroup<BitwiseOpParentheses>, DefaultIgnore;
 
+def warn_comp_op_in_comp_op : Warning<
+  "boolean value derived from '%0' is compared as the operand of '%1'">, InGroup<ComparisonOpParentheses>, DefaultIgnore;
+def warn_chaining_comparisons: Warning<
+  "boolean value derived from '%1' is compared as the operand of '%3'; did you mean '%0 %1 %2 && %2 %3 %4'?">, InGroup<ChainingComparisons>;
+def note_suggest_logical_and: Note<
+  "split the comparison into two">;
+
 def warn_logical_and_in_logical_or : Warning<
   "'&&' within '||'">, InGroup<LogicalOpParentheses>, DefaultIgnore;
 
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
   InGroup<OverloadedShiftOpParentheses>;
 def note_evaluate_comparison_first :Note<

clang/lib/Sema/SemaExpr.cpp

SuggestParentheses(S, OCE->getOperatorLoc(),
                      S.PDiag(diag::note_precedence_silence)
                          << (Kind == OO_LessLess ? "<<" : ">>"),
                      OCE->getSourceRange());
   SuggestParentheses(
       S, OpLoc, S.PDiag(diag::note_evaluate_comparison_first),
       SourceRange(OCE->getArg(1)->getBeginLoc(), RHSExpr->getEndLoc()));
 }
 
+/// EmitDiagnosticForCompOpInCompOp - 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.
+static void EmitDiagnosticForCompOpInCompOp(Sema &Self, SourceLocation OpLoc,
+                                            BinaryOperatorKind ParentOpc,
+                                            BinaryOperator *Bop) {
+  assert(Bop->isComparisonOp());
+  Self.Diag(Bop->getOperatorLoc(), diag::warn_comp_op_in_comp_op)
+      << Bop->getOpcodeStr() << BinaryOperator::getOpcodeStr(ParentOpc)
+      << Bop->getSourceRange() << OpLoc;
+
+  SuggestParentheses(Self, Bop->getOperatorLoc(),
+                     Self.PDiag(diag::note_precedence_silence)
+                         << Bop->getOpcodeStr(),
+                     Bop->getSourceRange());
+}
+
+/// EmitDiagnosticForChainingComparisons - Emit a diagnostic for a case
+/// where a relational operation is a direct sub-expression of another
+/// relational operation or where a `==` opration is of another `==`.
+/// Additionally, emit a fixit hint to suggest the
+/// addition of '&& <RHS>' to the end of the RHS of the inner BinaryOperator.
+static void EmitDiagnosticForChainingComparisons(Sema &Self,
+                                                 SourceLocation OpLoc,
+                                                 BinaryOperatorKind ParentOpc,
+                                                 Expr *SiblingExpr,
+                                                 BinaryOperator *Bop) {
+  assert(Bop->isRelationalOp() || Bop->getOpcode() == BO_EQ);
+
+  Self.Diag(Bop->getOperatorLoc(), diag::warn_chaining_comparisons)
+      << Bop->getLHS() << Bop->getOpcodeStr() << Bop->getRHS()
+      << BinaryOperator::getOpcodeStr(ParentOpc) << SiblingExpr
+      << Bop->getSourceRange() << OpLoc;
+
+  Expr *RHSExpr = Bop->getRHS();
+  CharSourceRange RHSExprRange = CharSourceRange::getCharRange(
+      RHSExpr->getBeginLoc(), Self.getLocForEndOfToken(RHSExpr->getEndLoc()));
+  StringRef RHSExprStrRef = Lexer::getSourceText(
+      RHSExprRange, Self.getSourceManager(), Self.getLangOpts());
+
+  SourceLocation InsertedLoc = Self.getLocForEndOfToken(Bop->getEndLoc());
+
+  Self.Diag(InsertedLoc, Self.PDiag(diag::note_suggest_logical_and))
+      << FixItHint::CreateInsertion(InsertedLoc, "&& ")
+      << FixItHint::CreateInsertion(InsertedLoc, RHSExprStrRef);
+}
+
+static void DiagnoseCompOpInCompOp(Sema &Self, SourceLocation OpLoc,
+                                   BinaryOperatorKind ParentOpc,
+                                   Expr *SiblingExpr, Expr *SubExpr) {
+  assert(BinaryOperator::isComparisonOp(ParentOpc));
+
+  BinaryOperator *SubBO = dyn_cast<BinaryOperator>(SubExpr);
+  if (!SubBO)
+    return;
+
+  // suggest inserting '&&' for chaining relational operators like 'a <= b < c'
+  // and for chaining `==` operators like 'a == b == c'
+  if ((BinaryOperator::isRelationalOp(ParentOpc) && SubBO->isRelationalOp()) ||
+      (ParentOpc == BO_EQ && SubBO->getOpcode() == BO_EQ))
+    EmitDiagnosticForChainingComparisons(Self, OpLoc, ParentOpc, SiblingExpr,
+                                         SubBO);
+  // suggest adding parentheses to clarify operator precedence for other
+  // chaining comparisons
+  else if (SubBO->isComparisonOp())
+    EmitDiagnosticForCompOpInCompOp(Self, OpLoc, ParentOpc, SubBO);
+}
+
 /// DiagnoseBinOpPrecedence - Emit warnings for expressions with tricky
 /// precedence.
 static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
                                     SourceLocation OpLoc, Expr *LHSExpr,
                                     Expr *RHSExpr){
   // Diagnose "arg1 'bitwise' arg2 'eq' arg3".
   if (BinaryOperator::isBitwiseOp(Opc))
     DiagnoseBitwisePrecedence(Self, Opc, OpLoc, LHSExpr, RHSExpr);
static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
 
   if ((Opc == BO_Shl && LHSExpr->getType()->isIntegralType(Self.getASTContext()))
       || Opc == BO_Shr) {
     StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
     DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
     DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
   }
 
+  if (BinaryOperator::isComparisonOp(Opc)) {
     // Warn on overloaded shift operators and comparisons, such as:
     // cout << 5 == 4;
-  if (BinaryOperator::isComparisonOp(Opc))
     DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);
+
+    // Warn on chaining comparisons like `a > b > c` and `a == b < c`
+    DiagnoseCompOpInCompOp(Self, OpLoc, Opc, RHSExpr, LHSExpr);
+    DiagnoseCompOpInCompOp(Self, OpLoc, Opc, LHSExpr, RHSExpr);
+  }
 }
 
 // Binary Operators.  'Tok' is the token for the operator.
 ExprResult Sema::ActOnBinOp(Scope *S, SourceLocation TokLoc,
                             tok::TokenKind Kind,
                             Expr *LHSExpr, Expr *RHSExpr) {
   BinaryOperatorKind Opc = ConvertTokenKindToBinaryOpcode(Kind);
   assert(LHSExpr && "ActOnBinOp(): missing left expression");

clang/test/Analysis/cxx-uninitialized-object.cpp

struct MultipleLambdaCapturesTest1 {
   Callable functor;
   int dontGetFilteredByNonPedanticMode = 0;
 
   MultipleLambdaCapturesTest1(const Callable &functor, int) : functor(functor) {} // expected-warning{{2 uninitialized field}}
 };
 
 void fMultipleLambdaCapturesTest1() {
   int b1, b2 = 3, b3;
-  auto equals = [&b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b1'}}
+  auto equals = [&b1, &b2, &b3](int a) { return ((a == b1) == b2) == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b1'}}
   // expected-note@-1{{uninitialized pointee 'this->functor./*captured variable*/b3'}}
   MultipleLambdaCapturesTest1<decltype(equals)>(equals, int());
 }
 
 template <class Callable>
 struct MultipleLambdaCapturesTest2 {
   Callable functor;
   int dontGetFilteredByNonPedanticMode = 0;
 
   MultipleLambdaCapturesTest2(const Callable &functor, int) : functor(functor) {} // expected-warning{{1 uninitialized field}}
 };
 
 void fMultipleLambdaCapturesTest2() {
   int b1, b2 = 3, b3;
-  auto equals = [b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b3'}}
+  auto equals = [b1, &b2, &b3](int a) { return ((a == b1) == b2) == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b3'}}
   MultipleLambdaCapturesTest2<decltype(equals)>(equals, int());
 }
 
 struct LambdaWrapper {
   void *func; // no-crash
   int dontGetFilteredByNonPedanticMode = 0;
 
   LambdaWrapper(void *ptr) : func(ptr) {} // expected-warning{{1 uninitialized field}}

clang/test/Misc/warning-wall.c

 CHECK-NEXT:    -Wcast-of-sel-type
 CHECK-NEXT:    -Wextern-c-compat
 CHECK-NEXT:    -Wuser-defined-warnings
 CHECK-NEXT:  -Wparentheses
 CHECK-NEXT:    -Wlogical-op-parentheses
 CHECK-NEXT:    -Wlogical-not-parentheses
 CHECK-NEXT:    -Wbitwise-conditional-parentheses
 CHECK-NEXT:    -Wbitwise-op-parentheses
+CHECK-NEXT:    -Wcomparison-op-parentheses
+CHECK-NEXT:    -Wchaining-comparisons
 CHECK-NEXT:    -Wshift-op-parentheses
 CHECK-NEXT:    -Woverloaded-shift-op-parentheses
 CHECK-NEXT:    -Wparentheses-equality
 CHECK-NEXT:    -Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
 CHECK-NEXT:  -Wmisleading-indentation
 
 
 CHECK-NOT:-W

clang/test/Sema/bool-compare.c

void f(int x, int y, int z) {
 
   if ((a<y) != 0) {} // no warning
   if ((a<y) != 1) {} // no warning
   if ((a<y) != 2) {} // expected-warning {{comparison of constant 2 with boolean expression is always true}}
   if ((a<y) != z) {} // no warning
   if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with boolean expression is always true}}
 
   if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // no warning
+  if (a>y<z)      {}
+  // expected-warning@-1 {{boolean value derived from '>' is compared as the operand of '<'; did you mean 'a > y && y < z'?}}
+  // expected-note@-2 {{split the comparison into two}}
   if ((a<y) > z)  {} // no warning
   if((a<y)>(z<y)) {} // no warning
   if((a<y)==(z<y)){} // no warning
   if((a<y)!=(z<y)){} // no warning
   if((z==x)<(y==z)){}// no warning
   if((a<y)!=((z==x)<(y==z))){} //no warning
 
 
void f(int x, int y, int z) {
 
   if (0 !=(a<y))  {} // no warning
   if (1 !=(a<y))  {} // no warning
   if (2 !=(a<y))  {} // expected-warning {{comparison of constant 2 with boolean expression is always true}}
   if (z !=(a<y))  {} // no warning
   if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with boolean expression is always true}}
 
   if (z ==(a<y))  {}    // no warning
-  if (z<a>y)      {}        // no warning
+  if (z<a>y)      {}
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '>'; did you mean 'z < a && a > y'?}}
+  // expected-note@-2 {{split the comparison into two}}
   if (z > (a<y))  {}    // no warning
   if((z<y)>(a<y)) {}   // no warning
   if((z<y)==(a<y)){}  // no warning
   if((z<y)!=(a<y)){}  // no warning
   if((y==z)<(z==x)){}  // no warning
   if(((z==x)<(y==z))!=(a<y)){}  // no warning
 
   if(((z==x)<(-1==z))!=(a<y)){} // no warning
   if(((z==x)<(z==-1))!=(a<y)){} // no warning
   if(((z==x)<-1)!=(a<y)){} // expected-warning {{comparison of constant -1 with boolean expression is always false}}
   if(((z==x)< 2)!=(a<y)){} // expected-warning {{comparison of constant 2 with boolean expression is always true}}
   if(((z==x)<(z>2))!=(a<y)){} // no warning
 
 }

clang/test/Sema/chaining-comparisons.c

+// RUN: %clang_cc1 -fsyntax-only -verify=off -Wno-chaining-comparisons %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wchaining-comparisons %s
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// off-no-diagnostics
+
+void comparison_op_parentheses(int a, int b, int c) {
+  (void)(a ==
+             b > c);
+
+  (void)(a ==b == c);
+  // expected-warning@-1 {{boolean value derived from '==' is compared as the operand of '=='; did you mean 'a == b && b == c'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:"&& b"
+
+  (void)(a !=b == c);
+
+  (void)(a !=
+             b < c);
+
+  (void)(a>=b >= c);
+  // expected-warning@-1 {{boolean value derived from '>=' is compared as the operand of '>='; did you mean 'a >= b && b >= c'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:14}:"&& b"
+
+  (void)(a >b >= c);
+  // expected-warning@-1 {{boolean value derived from '>' is compared as the operand of '>='; did you mean 'a > b && b >= c'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:14}:"&& b"
+
+  (void)(a >b > c);
+  // expected-warning@-1 {{boolean value derived from '>' is compared as the operand of '>'; did you mean 'a > b && b > c'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:14}:"&& b"
+
+  (void)(b + c <= c + a < a + b);
+  // expected-warning@-1 {{boolean value derived from '<=' is compared as the operand of '<'; did you mean 'b + c <= c + a && c + a < a + b'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:"&& c + a"
+
+
+  (void)(a<b <= c);
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '<='; did you mean 'a < b && b <= c'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:"&& b"
+
+  (void)(a<b > c);
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '>'; did you mean 'a < b && b > c'?}}
+  // expected-note@-2 {{split the comparison into two}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:"&& b"
+
+}

clang/test/Sema/comparison-op-parentheses.c

+// RUN: %clang_cc1 -fsyntax-only -verify=off %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wcomparison-op-parentheses
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wparentheses
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -Wcomparison-op-parentheses 2>&1 | FileCheck %s
+
+// off-no-diagnostics
+
+void comparison_op_parentheses(int a, int b, int c) {
+  (void)(a ==
+             b > c);
+  // expected-warning@-1 {{boolean value derived from '>' is compared as the operand of '=='}}
+  // expected-note@-2 {{place parentheses around the '>' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:14}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:19-[[@LINE-4]]:19}:")"
+
+  (void)(a !=b == c);
+  // expected-warning@-1 {{boolean value derived from '!=' is compared as the operand of '=='}}
+  // expected-note@-2 {{place parentheses around the '!=' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:15-[[@LINE-4]]:15}:")"
+
+  (void)(a !=
+             b < c);
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '!='}}
+  // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:14-[[@LINE-3]]:14}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:19-[[@LINE-4]]:19}:")"
+
+
+  (void)(a != (b > c));
+  (void)(a == (b > c));
+  (void)((a>b) >= c);
+  (void)((a<b) <= c);
+  (void)((a<b) > c);
+  (void)(a != b && a > c);
+  (void)((a<b) > c);
+}

clang/test/SemaCXX/bool-compare.cpp

void f(int x, int y, int z) {
 
   if ((a<y) != 0) {} // no warning
   if ((a<y) != 1) {} // no warning
   if ((a<y) != 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
   if ((a<y) != z) {} // no warning
   if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with expression of type 'bool' is always true}}
 
   if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // no warning
+  if (a>y<z)      {}
+  // expected-warning@-1 {{boolean value derived from '>' is compared as the operand of '<'; did you mean 'a > y && y < z'?}}
+  // expected-note@-2 {{split the comparison into two}}
   if ((a<y) > z)  {} // no warning
   if((a<y)>(z<y)) {} // no warning
   if((a<y)==(z<y)){} // no warning
   if((a<y)!=(z<y)){} // no warning
   if((z==x)<(y==z)){}  // no warning
   if((a<y)!=((z==x)<(y==z))){} // no warning
 
 
void f(int x, int y, int z) {
 
   if (0 !=(a<y))  {} // no warning
   if (1 !=(a<y))  {} // no warning
   if (2 !=(a<y))  {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
   if (z !=(a<y))  {} // no warning
   if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with expression of type 'bool' is always true}}
 
   if (z ==(a<y))  {} // no warning
-  if (z<a>y)      {} // no warning
+  if (z<a>y)      {}
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '>'; did you mean 'z < a && a > y'?}}
+  // expected-note@-2 {{split the comparison into two}}
   if (z > (a<y))  {} // no warning
   if((z<y)>(a<y)) {} // no warning
   if((z<y)==(a<y)){} // no warning
   if((z<y)!=(a<y)){} // no warning
   if((y==z)<(z==x)){} // no warning
   if(((z==x)<(y==z))!=(a<y)){}  // no warning
 
   if(((z==x)<(-1==z))!=(a<y)){} // no warning

clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp

 int d = g<int>(q);
 
 int e = h<0>(q); // ok, found by unqualified lookup
 
 void fn() {
   f<0>(q);
   int f;
   f<0>(q); // expected-error {{invalid operands to binary expression}}
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '>'; did you mean 'f < 0 && 0 > (q)'?}}
+  // expected-note@-2 {{split the comparison into two}}
 }
 
 void disambig() {
   // FIXME: It's unclear whether ending the template argument at the > inside the ?: is correct here (see DR579).
   f<true ? 1 > 2 : 3>(q); // expected-error {{expected ':'}} expected-note {{to match}} expected-error {{expected expression}}
 
   f < 1 + 3 > (q); // ok, function call
 }

clang/test/SemaTemplate/dependent-template-recover.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-chaining-comparisons %s
 template<typename T, typename U, int N>
 struct X {
   void f(T* t) {
     t->f0<U>(); // expected-error{{use 'template' keyword to treat 'f0' as a dependent template name}}
     t->f0<int>(); // expected-error{{use 'template' keyword to treat 'f0' as a dependent template name}}
 
     t->operator+<U const, 1>(1); // expected-error{{use 'template' keyword to treat 'operator +' as a dependent template name}}
     t->f1<int const, 2>(1); // expected-error{{use 'template' keyword to treat 'f1' as a dependent template name}}

clang/test/SemaTemplate/typo-dependent-name.cpp

struct X : Base<T> {
   template<int U>
   struct Inner {
   };
 
   bool f(T other) {
     // A pair of comparisons; 'inner' is a dependent name so can't be assumed
     // to be a template.
     return this->inner < other > ::z;
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '>'; did you mean 'this->inner < other && other > ::z'?}}
+  // expected-note@-2 {{split the comparison into two}}
   }
 };
 
 void use_x(X<int> x) { x.f(0); }
 
 template<typename T>
 struct Y {
   static int z;

clang/test/SemaTemplate/typo-template-name.cpp

void no_correction() {
 
     foo<int>(); // expected-error {{'foo' does not name a template but is followed by template arguments; did you mean 'Base::foo'?}}
     foo<>(); // expected-error {{'foo' does not name a template but is followed by template arguments; did you mean 'Base::foo'?}}
     foo<Base *>(); // expected-error {{'foo' does not name a template but is followed by template arguments; did you mean 'Base::foo'?}}
 
     // These are valid expressions.
     foo<foo; // expected-warning {{self-comparison}}
     foo<int()>(0);
+  // expected-warning@-1 {{boolean value derived from '<' is compared as the operand of '>'; did you mean 'foo < int() && int() > (0)'?}}
+  // expected-note@-2 {{split the comparison into two}}
     foo<int(), true>(false);
     foo<Base{}.n;
   }
 }

libcxx/test/support/test_comparisons.h

 }
 
 template <class Order, class T, class U = T>
 TEST_NODISCARD constexpr bool testOrder(const T& t1, const U& t2, Order order) {
     bool equal   = order == Order::equivalent;
     bool less    = order == Order::less;
     bool greater = order == Order::greater;
 
-    return (t1 <=> t2 == order) && testComparisonsComplete(t1, t2, equal, less, greater);
+    return ((t1 <=> t2) == order) && testComparisonsComplete(t1, t2, equal, less, greater);
 }
 
 template <class T, class Param>
 TEST_NODISCARD constexpr bool testOrderValues(Param val1, Param val2) {
   return testOrder(T(val1), T(val2), val1 <=> val2);
 }
 
 #endif