-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[Clang] Improve -Wtautological-overlap-compare
diagnostics flag
#133653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang-analysis Author: Yutong Zhu (YutongZhuu) ChangesThis PR attempts to improve the diagnostics flag Patch is 22.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133653.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2a1c5ee2d788e..e6a31072567e7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -256,6 +256,9 @@ Improvements to Clang's diagnostics
- Improve the diagnostics for shadows template parameter to report correct location (#GH129060).
+- Improved the ``-Wtautological-overlap-compare`` diagnostics to warn about overlapping and non-overlapping ranges involving character literals and floating-point literals.
+ The warning message for non-overlapping cases has also been improved (#GH13473).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 86c9c955c1c78..493a66df982c4 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10398,7 +10398,10 @@ def warn_tautological_negation_or_compare: Warning<
"'||' of a value and its negation always evaluates to true">,
InGroup<TautologicalNegationCompare>, DefaultIgnore;
def warn_tautological_overlap_comparison : Warning<
- "overlapping comparisons always evaluate to %select{false|true}0">,
+ "overlapping comparisons always evaluate to true">,
+ InGroup<TautologicalOverlapCompare>, DefaultIgnore;
+def warn_tautological_non_overlap_comparison : Warning<
+ "non-overlapping comparisons always evaluate to false">,
InGroup<TautologicalOverlapCompare>, DefaultIgnore;
def warn_depr_array_comparison : Warning<
"comparison between two arrays is deprecated; "
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 9af1e915482da..c6b7c49d4cb4a 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -70,13 +70,11 @@ static SourceLocation GetEndLoc(Decl *D) {
return D->getLocation();
}
-/// Returns true on constant values based around a single IntegerLiteral.
-/// Allow for use of parentheses, integer casts, and negative signs.
-/// FIXME: it would be good to unify this function with
-/// getIntegerLiteralSubexpressionValue at some point given the similarity
-/// between the functions.
+/// Returns true on constant values based around a single IntegerLiteral,
+/// CharacterLiteral, or FloatingLiteral. Allow for use of parentheses, integer
+/// casts, and negative signs.
-static bool IsIntegerLiteralConstantExpr(const Expr *E) {
+static bool IsLiteralConstantExpr(const Expr *E) {
// Allow parentheses
E = E->IgnoreParens();
@@ -93,16 +91,16 @@ static bool IsIntegerLiteralConstantExpr(const Expr *E) {
return false;
E = UO->getSubExpr();
}
-
- return isa<IntegerLiteral>(E);
+ return isa<IntegerLiteral>(E) || isa<CharacterLiteral>(E) ||
+ isa<FloatingLiteral>(E);
}
/// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
/// constant expression or EnumConstantDecl from the given Expr. If it fails,
/// returns nullptr.
-static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
+static const Expr *tryTransformToLiteralConstants(const Expr *E) {
E = E->IgnoreParens();
- if (IsIntegerLiteralConstantExpr(E))
+ if (IsLiteralConstantExpr(E))
return E;
if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
@@ -119,7 +117,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
BinaryOperatorKind Op = B->getOpcode();
const Expr *MaybeDecl = B->getLHS();
- const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS());
+ const Expr *Constant = tryTransformToLiteralConstants(B->getRHS());
// Expr looked like `0 == Foo` instead of `Foo == 0`
if (Constant == nullptr) {
// Flip the operator
@@ -133,7 +131,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
Op = BO_GE;
MaybeDecl = B->getRHS();
- Constant = tryTransformToIntOrEnumConstant(B->getLHS());
+ Constant = tryTransformToLiteralConstants(B->getLHS());
}
return std::make_tuple(MaybeDecl, Op, Constant);
@@ -1104,6 +1102,27 @@ class CFGBuilder {
}
}
+ TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
+ const llvm::APFloat &Value1,
+ const llvm::APFloat &Value2) {
+ switch (Relation) {
+ default:
+ return TryResult();
+ case BO_EQ:
+ return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpEqual);
+ case BO_NE:
+ return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpEqual);
+ case BO_LT:
+ return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpLessThan);
+ case BO_LE:
+ return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpGreaterThan);
+ case BO_GT:
+ return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpGreaterThan);
+ case BO_GE:
+ return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpLessThan);
+ }
+ }
+
/// There are two checks handled by this function:
/// 1. Find a law-of-excluded-middle or law-of-noncontradiction expression
/// e.g. if (x || !x), if (x && !x)
@@ -1171,81 +1190,135 @@ class CFGBuilder {
return {};
Expr::EvalResult L1Result, L2Result;
- if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
- !NumExpr2->EvaluateAsInt(L2Result, *Context))
- return {};
-
- llvm::APSInt L1 = L1Result.Val.getInt();
- llvm::APSInt L2 = L2Result.Val.getInt();
-
- // Can't compare signed with unsigned or with different bit width.
- if (L1.isSigned() != L2.isSigned() || L1.getBitWidth() != L2.getBitWidth())
+ llvm::APFloat L1ResultFloat(llvm::APFloat::IEEEdouble()),
+ L2ResultFloat(llvm::APFloat::IEEEdouble());
+ bool IsInt1 = NumExpr1->EvaluateAsInt(L1Result, *Context);
+ bool IsInt2 = NumExpr2->EvaluateAsInt(L2Result, *Context);
+ bool IsFloat1 = NumExpr1->EvaluateAsFloat(L1ResultFloat, *Context);
+ bool IsFloat2 = NumExpr2->EvaluateAsFloat(L2ResultFloat, *Context);
+
+ if ((!IsInt1 || !IsInt2) && (!IsFloat1 || !IsFloat2))
return {};
- // Values that will be used to determine if result of logical
- // operator is always true/false
- const llvm::APSInt Values[] = {
- // Value less than both Value1 and Value2
- llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()),
- // L1
- L1,
- // Value between Value1 and Value2
- ((L1 < L2) ? L1 : L2) + llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1),
- L1.isUnsigned()),
- // L2
- L2,
- // Value greater than both Value1 and Value2
- llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()),
- };
+ // Handle integer comparison
+ if (IsInt1 && IsInt2) {
+ llvm::APSInt L1 = L1Result.Val.getInt();
+ llvm::APSInt L2 = L2Result.Val.getInt();
- // Check whether expression is always true/false by evaluating the following
- // * variable x is less than the smallest literal.
- // * variable x is equal to the smallest literal.
- // * Variable x is between smallest and largest literal.
- // * Variable x is equal to the largest literal.
- // * Variable x is greater than largest literal.
- bool AlwaysTrue = true, AlwaysFalse = true;
- // Track value of both subexpressions. If either side is always
- // true/false, another warning should have already been emitted.
- bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
- bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
- for (const llvm::APSInt &Value : Values) {
- TryResult Res1, Res2;
- Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
- Res2 = analyzeLogicOperatorCondition(BO2, Value, L2);
-
- if (!Res1.isKnown() || !Res2.isKnown())
+ // Can't compare signed with unsigned or with different bit width.
+ if (L1.isSigned() != L2.isSigned() ||
+ L1.getBitWidth() != L2.getBitWidth())
return {};
- if (B->getOpcode() == BO_LAnd) {
- AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
- AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
- } else {
- AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
- AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
+ // Values that will be used to determine if result of logical
+ // operator is always true/false
+ const llvm::APSInt Values[] = {
+ // Value less than both Value1 and Value2
+ llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()),
+ // L1
+ L1,
+ // Value between Value1 and Value2
+ ((L1 < L2) ? L1 : L2) +
+ llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1), L1.isUnsigned()),
+ // L2
+ L2,
+ // Value greater than both Value1 and Value2
+ llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()),
+ };
+
+ // Check whether expression is always true/false by evaluating the
+ // following
+ // * variable x is less than the smallest literal.
+ // * variable x is equal to the smallest literal.
+ // * Variable x is between smallest and largest literal.
+ // * Variable x is equal to the largest literal.
+ // * Variable x is greater than largest literal.
+ bool AlwaysTrue = true, AlwaysFalse = true;
+ // Track value of both subexpressions. If either side is always
+ // true/false, another warning should have already been emitted.
+ bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
+ bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
+ for (const llvm::APSInt &Value : Values) {
+ TryResult Res1, Res2;
+ Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
+ Res2 = analyzeLogicOperatorCondition(BO2, Value, L2);
+
+ if (!Res1.isKnown() || !Res2.isKnown())
+ return {};
+
+ if (B->getOpcode() == BO_LAnd) {
+ AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
+ } else {
+ AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
+ }
+
+ LHSAlwaysTrue &= Res1.isTrue();
+ LHSAlwaysFalse &= Res1.isFalse();
+ RHSAlwaysTrue &= Res2.isTrue();
+ RHSAlwaysFalse &= Res2.isFalse();
}
- LHSAlwaysTrue &= Res1.isTrue();
- LHSAlwaysFalse &= Res1.isFalse();
- RHSAlwaysTrue &= Res2.isTrue();
- RHSAlwaysFalse &= Res2.isFalse();
+ if (AlwaysTrue || AlwaysFalse) {
+ if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
+ !RHSAlwaysFalse && BuildOpts.Observer)
+ BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
+ return TryResult(AlwaysTrue);
+ }
+ return {};
}
- if (AlwaysTrue || AlwaysFalse) {
- if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
- !RHSAlwaysFalse && BuildOpts.Observer)
- BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
- return TryResult(AlwaysTrue);
+ // Handle float comparison, the logic is similar to integer comparison
+ if (IsFloat1 && IsFloat2) {
+ llvm::APFloat MidValue = L1ResultFloat;
+ MidValue.add(L2ResultFloat, llvm::APFloat::rmNearestTiesToEven);
+ MidValue.divide(llvm::APFloat(2.0), llvm::APFloat::rmNearestTiesToEven);
+
+ const llvm::APFloat Values[] = {
+ llvm::APFloat::getSmallest(L1ResultFloat.getSemantics(), true),
+ L1ResultFloat,
+ MidValue,
+ L2ResultFloat,
+ llvm::APFloat::getLargest(L1ResultFloat.getSemantics(), false),
+ };
+
+ bool AlwaysTrue = true, AlwaysFalse = true;
+
+ for (const llvm::APFloat &Value : Values) {
+ TryResult Res1, Res2;
+ Res1 = analyzeLogicOperatorCondition(BO1, Value, L1ResultFloat);
+ Res2 = analyzeLogicOperatorCondition(BO2, Value, L2ResultFloat);
+
+ if (!Res1.isKnown() || !Res2.isKnown())
+ return {};
+
+ if (B->getOpcode() == BO_LAnd) {
+ AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
+ } else {
+ AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
+ }
+ }
+
+ if (AlwaysTrue || AlwaysFalse) {
+ if (BuildOpts.Observer)
+ BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
+ return TryResult(AlwaysTrue);
+ }
+ return {};
}
+
return {};
}
/// A bitwise-or with a non-zero constant always evaluates to true.
TryResult checkIncorrectBitwiseOrOperator(const BinaryOperator *B) {
const Expr *LHSConstant =
- tryTransformToIntOrEnumConstant(B->getLHS()->IgnoreParenImpCasts());
+ tryTransformToLiteralConstants(B->getLHS()->IgnoreParenImpCasts());
const Expr *RHSConstant =
- tryTransformToIntOrEnumConstant(B->getRHS()->IgnoreParenImpCasts());
+ tryTransformToLiteralConstants(B->getRHS()->IgnoreParenImpCasts());
if ((LHSConstant && RHSConstant) || (!LHSConstant && !RHSConstant))
return {};
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 3d6da4f70f99e..487b44d6fb312 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -166,13 +166,16 @@ class LogicalErrorHandler : public CFGCallback {
S.Diag(B->getExprLoc(), DiagID) << DiagRange;
}
- void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
+ void compareAlwaysTrue(const BinaryOperator *B,
+ bool isAlwaysTrueOrFalse) override {
if (HasMacroID(B))
return;
SourceRange DiagRange = B->getSourceRange();
- S.Diag(B->getExprLoc(), diag::warn_tautological_overlap_comparison)
- << DiagRange << isAlwaysTrue;
+ S.Diag(B->getExprLoc(),
+ isAlwaysTrueOrFalse ? diag::warn_tautological_overlap_comparison
+ : diag::warn_tautological_non_overlap_comparison)
+ << DiagRange;
}
void compareBitwiseEquality(const BinaryOperator *B,
diff --git a/clang/test/Sema/warn-overlap.c b/clang/test/Sema/warn-overlap.c
index 2db07ebcd17b8..e43c63bdaff9b 100644
--- a/clang/test/Sema/warn-overlap.c
+++ b/clang/test/Sema/warn-overlap.c
@@ -37,37 +37,37 @@ void f(int x) {
if (x >= 0 || x <= 0) { } // expected-warning {{overlapping comparisons always evaluate to true}}
// > && <
- if (x > 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 2 && x < 3) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 0 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x > 2 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 2 && x < 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 2 && x < 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 0 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
- if (x > 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 2 && x <= 2) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x > 2 && x <= 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 2 && x <= 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x > 2 && x <= 3) { }
- if (x >= 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x >= 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x >= 2 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x >= 2 && x < 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x >= 2 && x < 3) { }
- if (x >= 0 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x >= 0 && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
- if (x >= 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x >= 2 && x <= 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x >= 2 && x <= 2) { }
if (x >= 2 && x <= 3) { }
// !=, ==, ..
if (x != 2 || x != 3) { } // expected-warning {{overlapping comparisons always evaluate to true}}
if (x != 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}}
- if (x == 2 && x == 3) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x == 2 && x > 3) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x == 3 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (3 == x && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x == 2 && x == 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x == 2 && x > 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x == 3 && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (3 == x && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x == mydefine && x > 3) { }
if (x == (mydefine + 1) && x > 3) { }
if (x != CHOICE_0 || x != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}}
- if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
// Don't warn if comparing x to different types
if (x == CHOICE_0 && x == 1) { }
@@ -80,7 +80,7 @@ void f(int x) {
void enums(enum Choices c) {
if (c != CHOICE_0 || c != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}}
- if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
// Don't warn if comparing x to different types
if (c == CHOICE_0 && c == 1) { }
@@ -117,12 +117,12 @@ void assignment(int x) {
int a = x > 4 || x < 10;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
int b = x < 2 && x > 5;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
int c = x != 1 || x != 3;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
int d = x == 1 && x == 2;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
int e = x < 1 || x != 0;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
@@ -132,12 +132,12 @@ int returns(int x) {
return x > 4 || x < 10;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
return x < 2 && x > 5;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
return x != 1 || x != 3;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
return x == 1 && x == 2;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
return x < 1 || x != 0;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
@@ -169,7 +169,73 @@ int struct_test(struct A a) {
return a.x > 5 && a.y...
[truncated]
|
@llvm/pr-subscribers-clang Author: Yutong Zhu (YutongZhuu) ChangesThis PR attempts to improve the diagnostics flag Patch is 22.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133653.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2a1c5ee2d788e..e6a31072567e7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -256,6 +256,9 @@ Improvements to Clang's diagnostics
- Improve the diagnostics for shadows template parameter to report correct location (#GH129060).
+- Improved the ``-Wtautological-overlap-compare`` diagnostics to warn about overlapping and non-overlapping ranges involving character literals and floating-point literals.
+ The warning message for non-overlapping cases has also been improved (#GH13473).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 86c9c955c1c78..493a66df982c4 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10398,7 +10398,10 @@ def warn_tautological_negation_or_compare: Warning<
"'||' of a value and its negation always evaluates to true">,
InGroup<TautologicalNegationCompare>, DefaultIgnore;
def warn_tautological_overlap_comparison : Warning<
- "overlapping comparisons always evaluate to %select{false|true}0">,
+ "overlapping comparisons always evaluate to true">,
+ InGroup<TautologicalOverlapCompare>, DefaultIgnore;
+def warn_tautological_non_overlap_comparison : Warning<
+ "non-overlapping comparisons always evaluate to false">,
InGroup<TautologicalOverlapCompare>, DefaultIgnore;
def warn_depr_array_comparison : Warning<
"comparison between two arrays is deprecated; "
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 9af1e915482da..c6b7c49d4cb4a 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -70,13 +70,11 @@ static SourceLocation GetEndLoc(Decl *D) {
return D->getLocation();
}
-/// Returns true on constant values based around a single IntegerLiteral.
-/// Allow for use of parentheses, integer casts, and negative signs.
-/// FIXME: it would be good to unify this function with
-/// getIntegerLiteralSubexpressionValue at some point given the similarity
-/// between the functions.
+/// Returns true on constant values based around a single IntegerLiteral,
+/// CharacterLiteral, or FloatingLiteral. Allow for use of parentheses, integer
+/// casts, and negative signs.
-static bool IsIntegerLiteralConstantExpr(const Expr *E) {
+static bool IsLiteralConstantExpr(const Expr *E) {
// Allow parentheses
E = E->IgnoreParens();
@@ -93,16 +91,16 @@ static bool IsIntegerLiteralConstantExpr(const Expr *E) {
return false;
E = UO->getSubExpr();
}
-
- return isa<IntegerLiteral>(E);
+ return isa<IntegerLiteral>(E) || isa<CharacterLiteral>(E) ||
+ isa<FloatingLiteral>(E);
}
/// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
/// constant expression or EnumConstantDecl from the given Expr. If it fails,
/// returns nullptr.
-static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
+static const Expr *tryTransformToLiteralConstants(const Expr *E) {
E = E->IgnoreParens();
- if (IsIntegerLiteralConstantExpr(E))
+ if (IsLiteralConstantExpr(E))
return E;
if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
@@ -119,7 +117,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
BinaryOperatorKind Op = B->getOpcode();
const Expr *MaybeDecl = B->getLHS();
- const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS());
+ const Expr *Constant = tryTransformToLiteralConstants(B->getRHS());
// Expr looked like `0 == Foo` instead of `Foo == 0`
if (Constant == nullptr) {
// Flip the operator
@@ -133,7 +131,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
Op = BO_GE;
MaybeDecl = B->getRHS();
- Constant = tryTransformToIntOrEnumConstant(B->getLHS());
+ Constant = tryTransformToLiteralConstants(B->getLHS());
}
return std::make_tuple(MaybeDecl, Op, Constant);
@@ -1104,6 +1102,27 @@ class CFGBuilder {
}
}
+ TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
+ const llvm::APFloat &Value1,
+ const llvm::APFloat &Value2) {
+ switch (Relation) {
+ default:
+ return TryResult();
+ case BO_EQ:
+ return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpEqual);
+ case BO_NE:
+ return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpEqual);
+ case BO_LT:
+ return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpLessThan);
+ case BO_LE:
+ return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpGreaterThan);
+ case BO_GT:
+ return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpGreaterThan);
+ case BO_GE:
+ return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpLessThan);
+ }
+ }
+
/// There are two checks handled by this function:
/// 1. Find a law-of-excluded-middle or law-of-noncontradiction expression
/// e.g. if (x || !x), if (x && !x)
@@ -1171,81 +1190,135 @@ class CFGBuilder {
return {};
Expr::EvalResult L1Result, L2Result;
- if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
- !NumExpr2->EvaluateAsInt(L2Result, *Context))
- return {};
-
- llvm::APSInt L1 = L1Result.Val.getInt();
- llvm::APSInt L2 = L2Result.Val.getInt();
-
- // Can't compare signed with unsigned or with different bit width.
- if (L1.isSigned() != L2.isSigned() || L1.getBitWidth() != L2.getBitWidth())
+ llvm::APFloat L1ResultFloat(llvm::APFloat::IEEEdouble()),
+ L2ResultFloat(llvm::APFloat::IEEEdouble());
+ bool IsInt1 = NumExpr1->EvaluateAsInt(L1Result, *Context);
+ bool IsInt2 = NumExpr2->EvaluateAsInt(L2Result, *Context);
+ bool IsFloat1 = NumExpr1->EvaluateAsFloat(L1ResultFloat, *Context);
+ bool IsFloat2 = NumExpr2->EvaluateAsFloat(L2ResultFloat, *Context);
+
+ if ((!IsInt1 || !IsInt2) && (!IsFloat1 || !IsFloat2))
return {};
- // Values that will be used to determine if result of logical
- // operator is always true/false
- const llvm::APSInt Values[] = {
- // Value less than both Value1 and Value2
- llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()),
- // L1
- L1,
- // Value between Value1 and Value2
- ((L1 < L2) ? L1 : L2) + llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1),
- L1.isUnsigned()),
- // L2
- L2,
- // Value greater than both Value1 and Value2
- llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()),
- };
+ // Handle integer comparison
+ if (IsInt1 && IsInt2) {
+ llvm::APSInt L1 = L1Result.Val.getInt();
+ llvm::APSInt L2 = L2Result.Val.getInt();
- // Check whether expression is always true/false by evaluating the following
- // * variable x is less than the smallest literal.
- // * variable x is equal to the smallest literal.
- // * Variable x is between smallest and largest literal.
- // * Variable x is equal to the largest literal.
- // * Variable x is greater than largest literal.
- bool AlwaysTrue = true, AlwaysFalse = true;
- // Track value of both subexpressions. If either side is always
- // true/false, another warning should have already been emitted.
- bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
- bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
- for (const llvm::APSInt &Value : Values) {
- TryResult Res1, Res2;
- Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
- Res2 = analyzeLogicOperatorCondition(BO2, Value, L2);
-
- if (!Res1.isKnown() || !Res2.isKnown())
+ // Can't compare signed with unsigned or with different bit width.
+ if (L1.isSigned() != L2.isSigned() ||
+ L1.getBitWidth() != L2.getBitWidth())
return {};
- if (B->getOpcode() == BO_LAnd) {
- AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
- AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
- } else {
- AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
- AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
+ // Values that will be used to determine if result of logical
+ // operator is always true/false
+ const llvm::APSInt Values[] = {
+ // Value less than both Value1 and Value2
+ llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()),
+ // L1
+ L1,
+ // Value between Value1 and Value2
+ ((L1 < L2) ? L1 : L2) +
+ llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1), L1.isUnsigned()),
+ // L2
+ L2,
+ // Value greater than both Value1 and Value2
+ llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()),
+ };
+
+ // Check whether expression is always true/false by evaluating the
+ // following
+ // * variable x is less than the smallest literal.
+ // * variable x is equal to the smallest literal.
+ // * Variable x is between smallest and largest literal.
+ // * Variable x is equal to the largest literal.
+ // * Variable x is greater than largest literal.
+ bool AlwaysTrue = true, AlwaysFalse = true;
+ // Track value of both subexpressions. If either side is always
+ // true/false, another warning should have already been emitted.
+ bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
+ bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
+ for (const llvm::APSInt &Value : Values) {
+ TryResult Res1, Res2;
+ Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
+ Res2 = analyzeLogicOperatorCondition(BO2, Value, L2);
+
+ if (!Res1.isKnown() || !Res2.isKnown())
+ return {};
+
+ if (B->getOpcode() == BO_LAnd) {
+ AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
+ } else {
+ AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
+ }
+
+ LHSAlwaysTrue &= Res1.isTrue();
+ LHSAlwaysFalse &= Res1.isFalse();
+ RHSAlwaysTrue &= Res2.isTrue();
+ RHSAlwaysFalse &= Res2.isFalse();
}
- LHSAlwaysTrue &= Res1.isTrue();
- LHSAlwaysFalse &= Res1.isFalse();
- RHSAlwaysTrue &= Res2.isTrue();
- RHSAlwaysFalse &= Res2.isFalse();
+ if (AlwaysTrue || AlwaysFalse) {
+ if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
+ !RHSAlwaysFalse && BuildOpts.Observer)
+ BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
+ return TryResult(AlwaysTrue);
+ }
+ return {};
}
- if (AlwaysTrue || AlwaysFalse) {
- if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
- !RHSAlwaysFalse && BuildOpts.Observer)
- BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
- return TryResult(AlwaysTrue);
+ // Handle float comparison, the logic is similar to integer comparison
+ if (IsFloat1 && IsFloat2) {
+ llvm::APFloat MidValue = L1ResultFloat;
+ MidValue.add(L2ResultFloat, llvm::APFloat::rmNearestTiesToEven);
+ MidValue.divide(llvm::APFloat(2.0), llvm::APFloat::rmNearestTiesToEven);
+
+ const llvm::APFloat Values[] = {
+ llvm::APFloat::getSmallest(L1ResultFloat.getSemantics(), true),
+ L1ResultFloat,
+ MidValue,
+ L2ResultFloat,
+ llvm::APFloat::getLargest(L1ResultFloat.getSemantics(), false),
+ };
+
+ bool AlwaysTrue = true, AlwaysFalse = true;
+
+ for (const llvm::APFloat &Value : Values) {
+ TryResult Res1, Res2;
+ Res1 = analyzeLogicOperatorCondition(BO1, Value, L1ResultFloat);
+ Res2 = analyzeLogicOperatorCondition(BO2, Value, L2ResultFloat);
+
+ if (!Res1.isKnown() || !Res2.isKnown())
+ return {};
+
+ if (B->getOpcode() == BO_LAnd) {
+ AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
+ } else {
+ AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
+ AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
+ }
+ }
+
+ if (AlwaysTrue || AlwaysFalse) {
+ if (BuildOpts.Observer)
+ BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
+ return TryResult(AlwaysTrue);
+ }
+ return {};
}
+
return {};
}
/// A bitwise-or with a non-zero constant always evaluates to true.
TryResult checkIncorrectBitwiseOrOperator(const BinaryOperator *B) {
const Expr *LHSConstant =
- tryTransformToIntOrEnumConstant(B->getLHS()->IgnoreParenImpCasts());
+ tryTransformToLiteralConstants(B->getLHS()->IgnoreParenImpCasts());
const Expr *RHSConstant =
- tryTransformToIntOrEnumConstant(B->getRHS()->IgnoreParenImpCasts());
+ tryTransformToLiteralConstants(B->getRHS()->IgnoreParenImpCasts());
if ((LHSConstant && RHSConstant) || (!LHSConstant && !RHSConstant))
return {};
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 3d6da4f70f99e..487b44d6fb312 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -166,13 +166,16 @@ class LogicalErrorHandler : public CFGCallback {
S.Diag(B->getExprLoc(), DiagID) << DiagRange;
}
- void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
+ void compareAlwaysTrue(const BinaryOperator *B,
+ bool isAlwaysTrueOrFalse) override {
if (HasMacroID(B))
return;
SourceRange DiagRange = B->getSourceRange();
- S.Diag(B->getExprLoc(), diag::warn_tautological_overlap_comparison)
- << DiagRange << isAlwaysTrue;
+ S.Diag(B->getExprLoc(),
+ isAlwaysTrueOrFalse ? diag::warn_tautological_overlap_comparison
+ : diag::warn_tautological_non_overlap_comparison)
+ << DiagRange;
}
void compareBitwiseEquality(const BinaryOperator *B,
diff --git a/clang/test/Sema/warn-overlap.c b/clang/test/Sema/warn-overlap.c
index 2db07ebcd17b8..e43c63bdaff9b 100644
--- a/clang/test/Sema/warn-overlap.c
+++ b/clang/test/Sema/warn-overlap.c
@@ -37,37 +37,37 @@ void f(int x) {
if (x >= 0 || x <= 0) { } // expected-warning {{overlapping comparisons always evaluate to true}}
// > && <
- if (x > 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 2 && x < 3) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 0 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x > 2 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 2 && x < 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 2 && x < 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 0 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
- if (x > 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x > 2 && x <= 2) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x > 2 && x <= 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x > 2 && x <= 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x > 2 && x <= 3) { }
- if (x >= 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x >= 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x >= 2 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x >= 2 && x < 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x >= 2 && x < 3) { }
- if (x >= 0 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x >= 0 && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
- if (x >= 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x >= 2 && x <= 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x >= 2 && x <= 2) { }
if (x >= 2 && x <= 3) { }
// !=, ==, ..
if (x != 2 || x != 3) { } // expected-warning {{overlapping comparisons always evaluate to true}}
if (x != 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}}
- if (x == 2 && x == 3) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x == 2 && x > 3) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (x == 3 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}}
- if (3 == x && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x == 2 && x == 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x == 2 && x > 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (x == 3 && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
+ if (3 == x && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
if (x == mydefine && x > 3) { }
if (x == (mydefine + 1) && x > 3) { }
if (x != CHOICE_0 || x != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}}
- if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
// Don't warn if comparing x to different types
if (x == CHOICE_0 && x == 1) { }
@@ -80,7 +80,7 @@ void f(int x) {
void enums(enum Choices c) {
if (c != CHOICE_0 || c != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}}
- if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}}
+ if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}}
// Don't warn if comparing x to different types
if (c == CHOICE_0 && c == 1) { }
@@ -117,12 +117,12 @@ void assignment(int x) {
int a = x > 4 || x < 10;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
int b = x < 2 && x > 5;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
int c = x != 1 || x != 3;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
int d = x == 1 && x == 2;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
int e = x < 1 || x != 0;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
@@ -132,12 +132,12 @@ int returns(int x) {
return x > 4 || x < 10;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
return x < 2 && x > 5;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
return x != 1 || x != 3;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
return x == 1 && x == 2;
- // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ // expected-warning@-1{{non-overlapping comparisons always evaluate to false}}
return x < 1 || x != 0;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
@@ -169,7 +169,73 @@ int struct_test(struct A a) {
return a.x > 5 && a.y...
[truncated]
|
-Wtautological-overlap-compare
diagnostics to warn a…-Wtautological-overlap-compare
diagnostics flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is a nice improvement to this diagnostic. I think there is a bit of code duplication here and there though that we can get rid of.
132beed
to
ca795c3
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
6afd117
to
47a82b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Feel free to @ me to merge this once clang-format and CI are fine with it because from what I can tell you don’t have commit access.
47a82b2
to
d33498d
Compare
7e8a653
to
995fd47
Compare
… overlapping and non-overlapping ranges involving character literals and floating-point literals.
I believe this is ready :) @Sirraide |
It found something :-) https://github.com/sqlite/sqlite/blob/eb55814c62adecfcb586263c11e60fb24973b1ef/ext/misc/vfstrace.c#L584 (Reported in https://sqlite.org/forum/forumpost/744dffb4ca) |
Also:
Thanks a lot for the improvement! |
This PR attempts to improve the diagnostics flag
-Wtautological-overlap-compare
(#13473). I have added code to warn about float-point literals and character literals. I have also changed the warning message for the non-overlapping case to provide a more correct hint to the user.