Index: clang/docs/ReleaseNotes.rst =================================================================== --- clang/docs/ReleaseNotes.rst +++ clang/docs/ReleaseNotes.rst @@ -73,6 +73,9 @@ - Clang now warns by default for C++20 and later about deprecated capture of ``this`` with a capture default of ``=``. This warning can be disabled with ``-Wno-deprecated-this-capture``. +- Clang now warns on chained comparisons like ``x > y > z`` and ``x == y > z`` + with `-Wcomparison-op-parentheses` flag. This fixes + `Issue 60256 `_ Non-comprehensive list of changes in this release ------------------------------------------------- Index: clang/include/clang/Basic/DiagnosticGroups.td =================================================================== --- clang/include/clang/Basic/DiagnosticGroups.td +++ clang/include/clang/Basic/DiagnosticGroups.td @@ -405,6 +405,7 @@ def GlobalConstructors : DiagGroup<"global-constructors">; def BitwiseConditionalParentheses: DiagGroup<"bitwise-conditional-parentheses">; def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">; +def ComparisonOpParentheses: DiagGroup<"comparison-op-parentheses">; def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">; def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">; def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">; @@ -896,6 +897,7 @@ LogicalNotParentheses, BitwiseConditionalParentheses, BitwiseOpParentheses, + ComparisonOpParentheses, ShiftOpParentheses, OverloadedShiftOpParentheses, ParenthesesOnEquality, Index: clang/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- clang/include/clang/Basic/DiagnosticSemaKinds.td +++ clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6650,6 +6650,13 @@ def warn_bitwise_op_in_bitwise_op : Warning< "'%0' within '%1'">, InGroup, DefaultIgnore; +def warn_comp_op_in_comp_op : Warning< + "'%0' within '%1'">, InGroup, DefaultIgnore; +def warn_rel_op_in_rel_op : Warning< + "'%1' within '%3'; did you mean '%0 %1 %2 && %2 %3 %4'?">, InGroup, DefaultIgnore; +def note_suggest_logical_and: Note< + "split the comparison into two">; + def warn_logical_and_in_logical_or : Warning< "'&&' within '||'">, InGroup, DefaultIgnore; Index: clang/lib/Sema/SemaExpr.cpp =================================================================== --- clang/lib/Sema/SemaExpr.cpp +++ clang/lib/Sema/SemaExpr.cpp @@ -15548,6 +15548,70 @@ 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()); +} + +/// EmitDiagnosticForRelationalOpInRelationalOp - Emit a diagnostic for a case +/// where a relational operation is a direct sub-expression of another +/// relational operation. Additionally, emit a fixit hint to suggest the +/// addition of '&& ' to the end of the RHS of the inner BinaryOperator. +static void EmitDiagnosticForRelationalOpInRelationalOp( + Sema &Self, SourceLocation OpLoc, BinaryOperatorKind ParentOpc, + Expr *SiblingExpr, BinaryOperator *Bop) { + assert(Bop->isRelationalOp()); + + Self.Diag(Bop->getOperatorLoc(), diag::warn_rel_op_in_rel_op) + << 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 *ParentOtherHSExpr, Expr *SubExpr) { + assert(BinaryOperator::isComparisonOp(ParentOpc)); + + BinaryOperator *SubBO = dyn_cast(SubExpr); + if (!SubBO) + return; + + // suggest inserting '&&' for chaining relational operators like 'a <= b < c' + if (BinaryOperator::isRelationalOp(ParentOpc) && SubBO->isRelationalOp()) + EmitDiagnosticForRelationalOpInRelationalOp(Self, OpLoc, ParentOpc, + ParentOtherHSExpr, 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, @@ -15578,10 +15642,15 @@ DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift); } - // Warn on overloaded shift operators and comparisons, such as: - // cout << 5 == 4; - if (BinaryOperator::isComparisonOp(Opc)) + if (BinaryOperator::isComparisonOp(Opc)) { + // Warn on overloaded shift operators and comparisons, such as: + // cout << 5 == 4; DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr); + + // warnings for 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. Index: clang/test/Misc/warning-wall.c =================================================================== --- clang/test/Misc/warning-wall.c +++ clang/test/Misc/warning-wall.c @@ -92,6 +92,7 @@ CHECK-NEXT: -Wlogical-not-parentheses CHECK-NEXT: -Wbitwise-conditional-parentheses CHECK-NEXT: -Wbitwise-op-parentheses +CHECK-NEXT: -Wcomparison-op-parentheses CHECK-NEXT: -Wshift-op-parentheses CHECK-NEXT: -Woverloaded-shift-op-parentheses CHECK-NEXT: -Wparentheses-equality Index: clang/test/Sema/comparison-op-parentheses.c =================================================================== --- /dev/null +++ clang/test/Sema/comparison-op-parentheses.c @@ -0,0 +1,73 @@ +// 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 {{'>' within '=='}} + // 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 {{'==' within '=='}} + // 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 {{'!=' within '=='}} + // 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 {{'<' within '!='}} + // 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 {{'>=' within '>='; 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 {{'>' within '>='; 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 {{'>' within '>'; 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 {{'<=' within '<'; 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 c); + // expected-warning@-1 {{'<' within '>'; 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)); + (void)(a == (b > c)); + (void)((a>b) >= c); + (void)((a c); + (void)(a != b && a > c); + (void)((a c); +} Index: libcxx/test/support/test_comparisons.h =================================================================== --- libcxx/test/support/test_comparisons.h +++ libcxx/test/support/test_comparisons.h @@ -162,7 +162,7 @@ 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