diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -60,6 +60,8 @@ Improvements to Clang's diagnostics ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Clang now warns on chained comparisons like ``x > y > z`` and ``x == y > z``. This fixes + `Issue 60256 `_ Non-comprehensive list of changes in this release ------------------------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/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, diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6650,6 +6650,9 @@ 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_logical_and_in_logical_or : Warning< "'&&' within '||'">, InGroup, DefaultIgnore; diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -15534,6 +15534,37 @@ SourceRange(OCE->getArg(1)->getBeginLoc(), RHSExpr->getEndLoc())); } +/// It accepts a comparison expr that is inside a comparison one. +/// Emit a diagnostic together with a fixit hint that wraps the inner comparison +/// expression 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()); +} + +static void DiagnoseCompOpInCompOp(Sema &Self, SourceLocation OpLoc, + BinaryOperatorKind ParentOpc, Expr *LHSExpr, + Expr *RHSExpr) { + assert(BinaryOperator::isComparisonOp(ParentOpc)); + + BinaryOperator *LHSBO = dyn_cast(LHSExpr); + BinaryOperator *RHSBO = dyn_cast(RHSExpr); + + if (LHSBO && LHSBO->isComparisonOp()) + EmitDiagnosticForCompOpInCompOp(Self, OpLoc, ParentOpc, LHSBO); + if (RHSBO && RHSBO->isComparisonOp()) + EmitDiagnosticForCompOpInCompOp(Self, OpLoc, ParentOpc, RHSBO); +} + /// DiagnoseBinOpPrecedence - Emit warnings for expressions with tricky /// precedence. static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc, @@ -15564,10 +15595,14 @@ 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, LHSExpr, RHSExpr); + } } // Binary Operators. 'Tok' is the token for the operator. diff --git a/clang/test/Misc/warning-wall.c b/clang/test/Misc/warning-wall.c --- a/clang/test/Misc/warning-wall.c +++ b/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 diff --git a/clang/test/Sema/comparison-op-parentheses.c b/clang/test/Sema/comparison-op-parentheses.c new file mode 100644 --- /dev/null +++ b/clang/test/Sema/comparison-op-parentheses.c @@ -0,0 +1,96 @@ +// RUN: %clang_cc1 -fsyntax-only -verify %s -DSILENCE +// 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 + +#ifdef SILENCE +// expected-no-diagnostics +#endif + +void comparison_op_parentheses(int a, int b, int c) { + (void)(a == + b > c); +#ifndef SILENCE + // expected-warning@-2 {{'>' within '=='}} + // expected-note@-3 {{place parentheses around the '>' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:19-[[@LINE-6]]:19}:")" + + (void)(a ==b == c); +#ifndef SILENCE + // expected-warning@-2 {{'==' within '=='}} + // expected-note@-3 {{place parentheses around the '==' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")" + + (void)(a !=b == c); +#ifndef SILENCE + // expected-warning@-2 {{'!=' within '=='}} + // expected-note@-3 {{place parentheses around the '!=' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:15-[[@LINE-6]]:15}:")" + + (void)(a != + b < c); +#ifndef SILENCE + // expected-warning@-2 {{'<' within '!='}} + // expected-note@-3 {{place parentheses around the '<' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:14-[[@LINE-5]]:14}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:19-[[@LINE-6]]:19}:")" + + (void)(a>=b >= c); +#ifndef SILENCE + // expected-warning@-2 {{'>=' within '>='}} + // expected-note@-3 {{place parentheses around the '>=' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:14-[[@LINE-6]]:14}:")" + + (void)(a >b >= c); +#ifndef SILENCE + // expected-warning@-2 {{'>' within '>='}} + // expected-note@-3 {{place parentheses around the '>' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:14-[[@LINE-6]]:14}:")" + + (void)(a >b > c); +#ifndef SILENCE + // expected-warning@-2 {{'>' within '>'}} + // expected-note@-3 {{place parentheses around the '>' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:14-[[@LINE-6]]:14}:")" + + + + (void)(a c); +#ifndef SILENCE + // expected-warning@-2 {{'<' within '>'}} + // expected-note@-3 {{place parentheses around the '<' expression to silence this warning}} +#endif + // CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:10-[[@LINE-5]]:10}:"(" + // CHECK: fix-it:"{{.*}}":{[[@LINE-6]]:13-[[@LINE-6]]:13}:")" + + (void)(a != (b > c)); + (void)(a == (b > c)); + (void)((a>b) >= c); + (void)((a c); + (void)(a != b && a > c); + (void)((a c); + (void)((a (c > a)); + +}