Index: clang/docs/ReleaseNotes.rst =================================================================== --- clang/docs/ReleaseNotes.rst +++ clang/docs/ReleaseNotes.rst @@ -181,6 +181,12 @@ - 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 `_ Bug Fixes in This Version ------------------------- Index: clang/include/clang/Basic/DiagnosticGroups.td =================================================================== --- clang/include/clang/Basic/DiagnosticGroups.td +++ clang/include/clang/Basic/DiagnosticGroups.td @@ -403,6 +403,8 @@ 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">; @@ -895,6 +897,8 @@ LogicalNotParentheses, BitwiseConditionalParentheses, BitwiseOpParentheses, + ComparisonOpParentheses, + ChainingComparisons, 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< + "boolean value derived from '%0' is compared as the operand of '%1'">, InGroup, 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; +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 @@ -15572,6 +15572,75 @@ 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 '&& ' 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(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, @@ -15602,10 +15671,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); + + // 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. Index: clang/test/Analysis/cxx-uninitialized-object.cpp =================================================================== --- clang/test/Analysis/cxx-uninitialized-object.cpp +++ clang/test/Analysis/cxx-uninitialized-object.cpp @@ -861,7 +861,7 @@ 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(equals, int()); } @@ -876,7 +876,7 @@ 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(equals, int()); } Index: clang/test/Misc/warning-wall.c =================================================================== --- clang/test/Misc/warning-wall.c +++ clang/test/Misc/warning-wall.c @@ -92,6 +92,8 @@ 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 Index: clang/test/Sema/bool-compare.c =================================================================== --- clang/test/Sema/bool-compare.c +++ clang/test/Sema/bool-compare.c @@ -85,7 +85,9 @@ if ((ayy' is compared as the operand of '<'; did you mean 'a > y && y < z'?}} + // expected-note@-2 {{split the comparison into two}} if ((a z) {} // no warning if((a(zy) {} // no warning + if (zy) {} + // 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(a&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 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" + +} Index: clang/test/Sema/comparison-op-parentheses.c =================================================================== --- /dev/null +++ clang/test/Sema/comparison-op-parentheses.c @@ -0,0 +1,37 @@ +// 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 c); + (void)(a != b && a > c); + (void)((a c); +} Index: clang/test/SemaCXX/bool-compare.cpp =================================================================== --- clang/test/SemaCXX/bool-compare.cpp +++ clang/test/SemaCXX/bool-compare.cpp @@ -98,7 +98,9 @@ if ((ayy' is compared as the operand of '<'; did you mean 'a > y && y < z'?}} + // expected-note@-2 {{split the comparison into two}} if ((a z) {} // no warning if((a(zy) {} // no warning + if (zy) {} + // 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(a(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() { Index: clang/test/SemaTemplate/dependent-template-recover.cpp =================================================================== --- clang/test/SemaTemplate/dependent-template-recover.cpp +++ clang/test/SemaTemplate/dependent-template-recover.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify %s +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-chaining-comparisons %s template struct X { void f(T* t) { Index: clang/test/SemaTemplate/typo-dependent-name.cpp =================================================================== --- clang/test/SemaTemplate/typo-dependent-name.cpp +++ clang/test/SemaTemplate/typo-dependent-name.cpp @@ -21,6 +21,8 @@ // 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}} } }; Index: clang/test/SemaTemplate/typo-template-name.cpp =================================================================== --- clang/test/SemaTemplate/typo-template-name.cpp +++ clang/test/SemaTemplate/typo-template-name.cpp @@ -37,6 +37,8 @@ // These are valid expressions. foo(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(false); foo t2 == order) && testComparisonsComplete(t1, t2, equal, less, greater); + return ((t1 <=> t2) == order) && testComparisonsComplete(t1, t2, equal, less, greater); } template