[clang] Improved the ``-Wtautological-overlap-compare`` diagnostics to warn a… (PR #133653)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 30 12:59:51 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: Yutong Zhu (YutongZhuu)
<details>
<summary>Changes</summary>
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.
---
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:
- (modified) clang/docs/ReleaseNotes.rst (+3)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4-1)
- (modified) clang/lib/Analysis/CFG.cpp (+143-70)
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+6-3)
- (modified) clang/test/Sema/warn-overlap.c (+87-21)
``````````diff
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 at -1{{overlapping comparisons always evaluate to true}}
int b = x < 2 && x > 5;
- // expected-warning at -1{{overlapping comparisons always evaluate to false}}
+ // expected-warning at -1{{non-overlapping comparisons always evaluate to false}}
int c = x != 1 || x != 3;
// expected-warning at -1{{overlapping comparisons always evaluate to true}}
int d = x == 1 && x == 2;
- // expected-warning at -1{{overlapping comparisons always evaluate to false}}
+ // expected-warning at -1{{non-overlapping comparisons always evaluate to false}}
int e = x < 1 || x != 0;
// expected-warning at -1{{overlapping comparisons always evaluate to true}}
@@ -132,12 +132,12 @@ int returns(int x) {
return x > 4 || x < 10;
// expected-warning at -1{{overlapping comparisons always evaluate to true}}
return x < 2 && x > 5;
- // expected-warning at -1{{overlapping comparisons always evaluate to false}}
+ // expected-warning at -1{{non-overlapping comparisons always evaluate to false}}
return x != 1 || x != 3;
// expected-warning at -1{{overlapping comparisons always evaluate to true}}
return x == 1 && x == 2;
- // expected-warning at -1{{overlapping comparisons always evaluate to false}}
+ // expected-warning at -1{{non-overlapping comparisons always evaluate to false}}
return x < 1 || x != 0;
// expected-warning at -1{{overlapping comparisons always evaluate to true}}
@@ -169,7 +169,73 @@ int struct_test(struct A a) {
return a.x > 5 && a.y...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/133653
More information about the cfe-commits
mailing list