hazohelet updated this revision to Diff 494635.
hazohelet added a comment.
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.
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/Misc/warning-wall.c
clang/test/Sema/comparison-op-parentheses.c
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
@@ -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 <class T, class Param>
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<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]]:13-[[@LINE-3]]:13}:"&& 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]]:13-[[@LINE-3]]:13}:"&& b"
+
+ (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/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/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 '&& <RHS>' 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<BinaryOperator>(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/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<
+ "'%0' within '%1'">, InGroup<ComparisonOpParentheses>, DefaultIgnore;
+def warn_rel_op_in_rel_op : Warning<
+ "'%1' within '%3'; did you mean '%0 %1 %2 && %2 %3 %4'?">, InGroup<ComparisonOpParentheses>, DefaultIgnore;
+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
@@ -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/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 <https://github.com/llvm/llvm-project/issues/60256>`_
Non-comprehensive list of changes in this release
-------------------------------------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
