vabridgers updated this revision to Diff 282471.
vabridgers marked 2 inline comments as done.
vabridgers added a comment.
Address simpler issues brought up during review so far.
Tests to be refactored, and a fixit added.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85097/new/
https://reviews.llvm.org/D85097
Files:
clang/docs/DiagnosticsReference.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/warn-compare-op-parentheses.c
Index: clang/test/Sema/warn-compare-op-parentheses.c
===================================================================
--- /dev/null
+++ clang/test/Sema/warn-compare-op-parentheses.c
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -fsyntax-only -Wcompare-op-parentheses -verify %s
+
+int case1(int p1, int p2, int p3) {
+ if (p1 < p2 < p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case2(int p1, int p2, int p3) {
+ // no warning
+ if ((p1 < p2) && (p2 < p3))
+ return 1;
+ return 0;
+}
+
+int case3(int p1, int p2, int p3) {
+ // no warning
+ if ((p1 < p2) && (p2))
+ return 1;
+ return 0;
+}
+
+int case4(int p1, int p2, int p3) {
+ // no warning
+ if ((p1) && (p3 < p2))
+ return 1;
+ return 0;
+}
+
+int case5(int p1, int p2, int p3) {
+ while (p1 < p3 < p2)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case6(int p1, int p2, int p3) {
+ // should not warn
+ while (p1 && p3 < p2)
+ return 1;
+ return 0;
+}
+
+int case7(int p1, int p2, int p3) {
+ // should not warn
+ while ((p1 < p3) < p2)
+ return 1;
+ return 0;
+}
+
+int case8(int p1, int p2, int p3) {
+ int ret = p1 < p2 < p3;
+ // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+ return ret;
+}
+
+int case9(int p1, int p2, int p3) {
+ int ret = (p1 < p2) < p3 < p1;
+ // expected-warning@-1 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-2 {{place parentheses around the '<' expression to silence this warning}}
+ return ret;
+}
+
+int case10(int p1, int p2, int p3) {
+ if (p1 <= p2 < p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case11(int p1, int p2, int p3) {
+ if (p1 < p2 <= p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '<' expression to silence this warning}}
+}
+
+int case12(int p1, int p2, int p3) {
+ if (p1 <= p2 <= p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '<=' expression to silence this warning}}
+}
+
+int case13(int p1, int p2, int p3) {
+ if (p1 > p2 < p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case14(int p1, int p2, int p3) {
+ if (p1 > p2 > p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '>' expression to silence this warning}}
+}
+
+int case15(int p1, int p2, int p3) {
+ if (p1 >= p2 > p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case16(int p1, int p2, int p3) {
+ if (p1 >= p2 >= p3)
+ return 1;
+ return 0;
+ // expected-warning@-3 {{comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'}}
+ // expected-note@-4 {{place parentheses around the '>=' expression to silence this warning}}
+}
+
+int case17(int p1, int p2, int p3) {
+ // should not warn
+ if (p1 >= p2 || p3)
+ return 1;
+ return 0;
+}
Index: clang/test/Misc/warning-wall.c
===================================================================
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -1,99 +1,9 @@
-RUN: diagtool tree -Wall > %t 2>&1
-RUN: FileCheck --input-file=%t %s
+RUN : diagtool tree - Wall > % t 2 > & 1 RUN : FileCheck-- input - file = % t % s
- CHECK:-Wall
-CHECK-NEXT: -Wmost
-CHECK-NEXT: -Wchar-subscripts
-CHECK-NEXT: -Wcomment
-CHECK-NEXT: -Wdelete-non-virtual-dtor
-CHECK-NEXT: -Wdelete-non-abstract-non-virtual-dtor
-CHECK-NEXT: -Wdelete-abstract-non-virtual-dtor
-CHECK-NEXT: -Wformat
-CHECK-NEXT: -Wformat-extra-args
-CHECK-NEXT: -Wformat-zero-length
-CHECK-NEXT: -Wnonnull
-CHECK-NEXT: -Wformat-security
-CHECK-NEXT: -Wformat-y2k
-CHECK-NEXT: -Wformat-invalid-specifier
-CHECK-NEXT: -Wfor-loop-analysis
-CHECK-NEXT: -Wframe-address
-CHECK-NEXT: -Wimplicit
-CHECK-NEXT: -Wimplicit-function-declaration
-CHECK-NEXT: -Wimplicit-int
-CHECK-NEXT: -Winfinite-recursion
-CHECK-NEXT: -Wint-in-bool-context
-CHECK-NEXT: -Wmismatched-tags
-CHECK-NEXT: -Wmissing-braces
-CHECK-NEXT: -Wmove
-CHECK-NEXT: -Wpessimizing-move
-CHECK-NEXT: -Wredundant-move
-CHECK-NEXT: -Wreturn-std-move
-CHECK-NEXT: -Wself-move
-CHECK-NEXT: -Wmultichar
-CHECK-NEXT: -Wrange-loop-construct
-CHECK-NEXT: -Wreorder
-CHECK-NEXT: -Wreorder-ctor
-CHECK-NEXT: -Wreorder-init-list
-CHECK-NEXT: -Wreturn-type
-CHECK-NEXT: -Wreturn-type-c-linkage
-CHECK-NEXT: -Wself-assign
-CHECK-NEXT: -Wself-assign-overloaded
-CHECK-NEXT: -Wself-assign-field
-CHECK-NEXT: -Wself-move
-CHECK-NEXT: -Wsizeof-array-argument
-CHECK-NEXT: -Wsizeof-array-decay
-CHECK-NEXT: -Wstring-plus-int
-CHECK-NEXT: -Wtautological-compare
-CHECK-NEXT: -Wtautological-constant-compare
-CHECK-NEXT: -Wtautological-constant-out-of-range-compare
-CHECK-NEXT: -Wtautological-pointer-compare
-CHECK-NEXT: -Wtautological-overlap-compare
-CHECK-NEXT: -Wtautological-bitwise-compare
-CHECK-NEXT: -Wtautological-undefined-compare
-CHECK-NEXT: -Wtautological-objc-bool-compare
-CHECK-NEXT: -Wtrigraphs
-CHECK-NEXT: -Wuninitialized
-CHECK-NEXT: -Wsometimes-uninitialized
-CHECK-NEXT: -Wstatic-self-init
-CHECK-NEXT: -Wuninitialized-const-reference
-CHECK-NEXT: -Wunknown-pragmas
-CHECK-NEXT: -Wunused
-CHECK-NEXT: -Wunused-argument
-CHECK-NEXT: -Wunused-function
-CHECK-NEXT: -Wunneeded-internal-declaration
-CHECK-NEXT: -Wunused-label
-CHECK-NEXT: -Wunused-private-field
-CHECK-NEXT: -Wunused-lambda-capture
-CHECK-NEXT: -Wunused-local-typedef
-CHECK-NEXT: -Wunused-value
-CHECK-NEXT: -Wunused-comparison
-CHECK-NEXT: -Wunused-result
-CHECK-NEXT: -Wunevaluated-expression
-CHECK-NEXT: -Wpotentially-evaluated-expression
-CHECK-NEXT: -Wunused-variable
-CHECK-NEXT: -Wunused-const-variable
-CHECK-NEXT: -Wunused-property-ivar
-CHECK-NEXT: -Wvolatile-register-var
-CHECK-NEXT: -Wobjc-missing-super-calls
-CHECK-NEXT: -Wobjc-designated-initializers
-CHECK-NEXT: -Wobjc-flexible-array
-CHECK-NEXT: -Woverloaded-virtual
-CHECK-NEXT: -Wprivate-extern
-CHECK-NEXT: -Wcast-of-sel-type
-CHECK-NEXT: -Wextern-c-compat
-CHECK-NEXT: -Wuser-defined-warnings
-CHECK-NEXT: -Wparentheses
-CHECK-NEXT: -Wlogical-op-parentheses
-CHECK-NEXT: -Wlogical-not-parentheses
-CHECK-NEXT: -Wbitwise-conditional-parentheses
-CHECK-NEXT: -Wbitwise-op-parentheses
-CHECK-NEXT: -Wshift-op-parentheses
-CHECK-NEXT: -Woverloaded-shift-op-parentheses
-CHECK-NEXT: -Wparentheses-equality
-CHECK-NEXT: -Wdangling-else
-CHECK-NEXT: -Wswitch
-CHECK-NEXT: -Wswitch-bool
-CHECK-NEXT: -Wmisleading-indentation
+ CHECK : -Wall CHECK -
+ NEXT : -Wmost
+ CHECK -
+ NEXT : -Wchar - subscripts CHECK - NEXT : -Wcomment CHECK - NEXT : -Wdelete - non - virtual - dtor CHECK - NEXT : -Wdelete - non - abstract - non - virtual - dtor CHECK - NEXT : -Wdelete - abstract - non - virtual - dtor CHECK - NEXT : -Wformat CHECK - NEXT : -Wformat - extra - args CHECK - NEXT : -Wformat - zero - length CHECK - NEXT : -Wnonnull CHECK - NEXT : -Wformat - security CHECK - NEXT : -Wformat - y2k CHECK - NEXT : -Wformat - invalid - specifier CHECK - NEXT : -Wfor - loop - analysis CHECK - NEXT : -Wframe - address CHECK - NEXT : -Wimplicit CHECK - NEXT : -Wimplicit - function - declaration CHECK - NEXT : -Wimplicit - int CHECK - NEXT : -Winfinite - recursion CHECK - NEXT : -Wint - in - bool - context CHECK - NEXT : -Wmismatched - tags CHECK - NEXT : -Wmissing - braces CHECK - NEXT : -Wmove CHECK - NEXT : -Wpessimizing - move CHECK - NEXT : -Wredundant - move CHECK - NEXT : -Wreturn - std - move CHECK - NEXT : -Wself - move CHECK - NEXT : -Wmultichar CHECK - NEXT : -Wrange - loop - construct CHECK - NEXT : -Wreorder CHECK - NEXT : -Wreorder - ctor CHECK - NEXT : -Wreorder - init - list CHECK - NEXT : -Wreturn - type CHECK - NEXT : -Wreturn - type - c - linkage CHECK - NEXT : -Wself - assign CHECK - NEXT : -Wself - assign - overloaded CHECK - NEXT : -Wself - assign - field CHECK - NEXT : -Wself - move CHECK - NEXT : -Wsizeof - array - argument CHECK - NEXT : -Wsizeof - array - decay CHECK - NEXT : -Wstring - plus - int CHECK - NEXT : -Wtautological - compare CHECK - NEXT : -Wtautological - constant - compare CHECK - NEXT : -Wtautological - constant - out - of - range - compare CHECK - NEXT : -Wtautological - pointer - compare CHECK - NEXT : -Wtautological - overlap - compare CHECK - NEXT : -Wtautological - bitwise - compare CHECK - NEXT : -Wtautological - undefined - compare CHECK - NEXT : -Wtautological - objc - bool - compare CHECK - NEXT : -Wtrigraphs CHECK - NEXT : -Wuninitialized CHECK - NEXT : -Wsometimes - uninitialized CHECK - NEXT : -Wstatic - self - init CHECK - NEXT : -Wuninitialized - const - reference CHECK - NEXT : -Wunknown - pragmas CHECK - NEXT : -Wunused CHECK - NEXT : -Wunused - argument CHECK - NEXT : -Wunused - function CHECK - NEXT : -Wunneeded - internal - declaration CHECK - NEXT : -Wunused - label CHECK - NEXT : -Wunused - private - field CHECK - NEXT : -Wunused - lambda - capture CHECK - NEXT : -Wunused - local - typedef CHECK - NEXT : -Wunused - value CHECK - NEXT : -Wunused - comparison CHECK - NEXT : -Wunused - result CHECK - NEXT : -Wunevaluated - expression CHECK - NEXT : -Wpotentially - evaluated - expression CHECK - NEXT : -Wunused - variable CHECK - NEXT : -Wunused - const - variable CHECK - NEXT : -Wunused - property - ivar CHECK - NEXT : -Wvolatile - register - var CHECK - NEXT : -Wobjc - missing - super - calls CHECK - NEXT : -Wobjc - designated - initializers CHECK - NEXT : -Wobjc - flexible - array CHECK - NEXT : -Woverloaded - virtual CHECK - NEXT : -Wprivate - extern CHECK - NEXT : -Wcast - of - sel - type CHECK - NEXT : -Wextern - c - compat CHECK - NEXT : -Wuser - defined - warnings CHECK - NEXT : -Wparentheses CHECK - NEXT : -Wcompare - op - parentheses CHECK - NEXT : -Wlogical - op - parentheses CHECK - NEXT : -Wlogical - not -parentheses CHECK - NEXT : -Wbitwise - conditional - parentheses CHECK - NEXT : -Wbitwise - op - parentheses CHECK - NEXT : -Wshift - op - parentheses CHECK - NEXT : -Woverloaded - shift - op - parentheses CHECK - NEXT : -Wparentheses - equality CHECK - NEXT : -Wdangling - else CHECK - NEXT : -Wswitch CHECK - NEXT : -Wswitch - bool CHECK - NEXT : -Wmisleading - indentation
-
-CHECK-NOT:-W
+ CHECK -
+ NOT : -W
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14002,6 +14002,17 @@
ParensRange);
}
+static void EmitDiagnosticForAmbiguousComparison(Sema &Self,
+ SourceLocation OpLoc,
+ BinaryOperator *Bop) {
+ Self.Diag(Bop->getOperatorLoc(), diag::warn_comparison_parentheses)
+ << Bop->getSourceRange() << OpLoc;
+ SuggestParentheses(Self, Bop->getOperatorLoc(),
+ Self.PDiag(diag::note_precedence_silence)
+ << Bop->getOpcodeStr(),
+ Bop->getSourceRange());
+}
+
/// It accepts a '&&' expr that is inside a '||' one.
/// Emit a diagnostic together with a fixit hint that wraps the '&&' expression
/// in parentheses.
@@ -14033,6 +14044,27 @@
E->EvaluateAsBooleanCondition(Res, S.getASTContext()) && !Res;
}
+static bool isComparisonOpSamePrecedence(BinaryOperatorKind Opc) {
+ switch (Opc) {
+ case BO_GT:
+ case BO_GE:
+ case BO_LT:
+ case BO_LE:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static void DiagnoseAmbiguousComparison(Sema &S, SourceLocation OpLoc,
+ Expr *LHSExpr, Expr *RHSExpr) {
+ if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(LHSExpr)) {
+ if (isComparisonOpSamePrecedence(Bop->getOpcode())) {
+ return EmitDiagnosticForAmbiguousComparison(S, OpLoc, Bop);
+ }
+ }
+}
+
/// Look for '&&' in the left hand of a '||' expr.
static void DiagnoseLogicalAndInLogicalOrLHS(Sema &S, SourceLocation OpLoc,
Expr *LHSExpr, Expr *RHSExpr) {
@@ -14162,6 +14194,9 @@
// cout << 5 == 4;
if (BinaryOperator::isComparisonOp(Opc))
DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);
+
+ if (isComparisonOpSamePrecedence(Opc))
+ DiagnoseAmbiguousComparison(Self, OpLoc, 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
@@ -6130,6 +6130,10 @@
def warn_bitwise_op_in_bitwise_op : Warning<
"'%0' within '%1'">, InGroup<BitwiseOpParentheses>, DefaultIgnore;
+def warn_comparison_parentheses : Warning<
+ "chained comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'">,
+ InGroup<CompareOpParentheses>, DefaultIgnore;
+
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
@@ -341,6 +341,7 @@
def GlobalConstructors : DiagGroup<"global-constructors">;
def BitwiseConditionalParentheses: DiagGroup<"bitwise-conditional-parentheses">;
def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
+def CompareOpParentheses: DiagGroup<"compare-op-parentheses">;
def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
@@ -785,7 +786,8 @@
// do not want these warnings.
def ParenthesesOnEquality : DiagGroup<"parentheses-equality">;
def Parentheses : DiagGroup<"parentheses",
- [LogicalOpParentheses,
+ [CompareOpParentheses,
+ LogicalOpParentheses,
LogicalNotParentheses,
BitwiseConditionalParentheses,
BitwiseOpParentheses,
Index: clang/docs/DiagnosticsReference.rst
===================================================================
--- clang/docs/DiagnosticsReference.rst
+++ clang/docs/DiagnosticsReference.rst
@@ -2850,6 +2850,15 @@
+---------------------------------------------------------------------------+
+-Wcompare-op-parentheses
+--------------------------------
+**Diagnostic text:**
+
++----------------------------------------------------------------------------------------------------------+
+|:warning:`warning:` |nbsp| :diagtext:`comparisons like 'x<=y<=z' are interpreted as '(x<=y ? 1 : 0) <= z'`|
++----------------------------------------------------------------------------------------------------------+
+
+
-Wcomplex-component-init
------------------------
**Diagnostic text:**
@@ -9873,7 +9882,7 @@
-------------
Some of the diagnostics controlled by this flag are enabled by default.
-Also controls `-Wbitwise-conditional-parentheses`_, `-Wbitwise-op-parentheses`_, `-Wdangling-else`_, `-Wlogical-not-parentheses`_, `-Wlogical-op-parentheses`_, `-Woverloaded-shift-op-parentheses`_, `-Wparentheses-equality`_, `-Wshift-op-parentheses`_.
+Also controls `-Wbitwise-conditional-parentheses`_, `-Wbitwise-op-parentheses`_, `-Wcompare-op-parentheses`_, `-Wdangling-else`_, `-Wlogical-not-parentheses`_, `-Wlogical-op-parentheses`_, `-Woverloaded-shift-op-parentheses`_, `-Wparentheses-equality`_, `-Wshift-op-parentheses`_.
**Diagnostic text:**
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
