hazohelet updated this revision to Diff 506887.
hazohelet added a comment.
Herald added a subscriber: jplehr.
Rebase and Ping
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142800/new/
https://reviews.llvm.org/D142800
Files:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Analysis/cxx-uninitialized-object.cpp
clang/test/Misc/warning-wall.c
clang/test/Sema/bool-compare.c
clang/test/Sema/chaining-comparisons.c
clang/test/Sema/comparison-op-parentheses.c
clang/test/SemaCXX/bool-compare.cpp
clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
clang/test/SemaTemplate/dependent-template-recover.cpp
clang/test/SemaTemplate/typo-dependent-name.cpp
clang/test/SemaTemplate/typo-template-name.cpp
libcxx/test/support/test_comparisons.h
Index: libcxx/test/support/test_comparisons.h
===================================================================
--- libcxx/test/support/test_comparisons.h
+++ libcxx/test/support/test_comparisons.h
@@ -164,7 +164,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 <class T, class Param>
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<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;
}
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/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<typename T, typename U, int N>
struct X {
void f(T* t) {
Index: clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
===================================================================
--- clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
+++ clang/test/SemaCXX/cxx2a-adl-only-template-id.cpp
@@ -28,6 +28,8 @@
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() {
Index: clang/test/SemaCXX/bool-compare.cpp
===================================================================
--- clang/test/SemaCXX/bool-compare.cpp
+++ clang/test/SemaCXX/bool-compare.cpp
@@ -98,7 +98,9 @@
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
@@ -159,7 +161,9 @@
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
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<b) <= c);
+ (void)((a<b) > c);
+ (void)(a != b && a > c);
+ (void)((a<b) > c);
+}
Index: clang/test/Sema/chaining-comparisons.c
===================================================================
--- /dev/null
+++ clang/test/Sema/chaining-comparisons.c
@@ -0,0 +1,53 @@
+// 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"
+
+}
Index: clang/test/Sema/bool-compare.c
===================================================================
--- clang/test/Sema/bool-compare.c
+++ clang/test/Sema/bool-compare.c
@@ -85,7 +85,9 @@
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
@@ -145,7 +147,9 @@
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
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/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<decltype(equals)>(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<decltype(equals)>(equals, int());
}
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 '&& <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,
@@ -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/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<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;
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/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 <https://github.com/llvm/llvm-project/issues/60256>`_
Bug Fixes in This Version
-------------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
