[clang-tools-extra] [clang-tidy] Improve integer comparison by matching valid expressions outside implicitCastExpr for use-integer-sign-comparison (PR #144240)
David Rivera via cfe-commits
cfe-commits at lists.llvm.org
Sat Jun 14 17:32:32 PDT 2025
https://github.com/RiverDave created https://github.com/llvm/llvm-project/pull/144240
Expands check based on: https://github.com/llvm/llvm-project/issues/127471
What changed:
1. I’m detecting `callExpr` not directly wrapped by an implicit cast. where its `sourceExpression` is of integer type.
2. The check originally counted with the fact that signed integer expressions would have an implicit cast expression for all **comparison cases**, but that doesn’t seem to be correct as shown below.
(For the following snippet. as referenced in #143927)
```cpp
struct A
{
unsigned size() const;
};
struct B
{
A a;
bool foo(const A& a2) const { return (int)a2.size() == size(); //here }
int size() const { return (int)a.size(); }
};
```
(see AST):
<img width="881" alt="Screenshot 2025-06-14 at 7 24 20 PM" src="https://github.com/user-attachments/assets/d112b7c3-2217-4195-bfc4-cf4c5b46abd4" />
**Notice how the signed operand is not wrapped by a cast in any way.**
>From 4e78c29eb5f2d7d750c467b014a6827c8db9f19c Mon Sep 17 00:00:00 2001
From: David Rivera <davidriverg at gmail.com>
Date: Wed, 2 Apr 2025 21:02:00 -0400
Subject: [PATCH] [clang-tidy] Improve integer comparison by matching valid
expressions outside implicitCastExpr for use-integer-sign-comparison
---
.../UseIntegerSignComparisonCheck.cpp | 38 +++++---
clang-tools-extra/docs/ReleaseNotes.rst | 4 +
.../modernize/use-integer-sign-comparison.cpp | 97 +++++++++++++++++++
3 files changed, 123 insertions(+), 16 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
index eeba5cce80da5..8f0bcf26e8357 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
@@ -39,21 +39,23 @@ intCastExpression(bool IsSigned,
// std::cmp_{} functions trigger a compile-time error if either LHS or RHS
// is a non-integer type, char, enum or bool
// (unsigned char/ signed char are Ok and can be used).
- auto IntTypeExpr = expr(hasType(hasCanonicalType(qualType(
+ const auto HasIntegerType = hasType(hasCanonicalType(qualType(
isInteger(), IsSigned ? isSignedInteger() : isUnsignedInteger(),
- unless(isActualChar()), unless(booleanType()), unless(enumType())))));
+ unless(isActualChar()), unless(booleanType()), unless(enumType()))));
+
+ const auto IntTypeExpr = expr(HasIntegerType);
const auto ImplicitCastExpr =
- CastBindName.empty() ? implicitCastExpr(hasSourceExpression(IntTypeExpr))
- : implicitCastExpr(hasSourceExpression(IntTypeExpr))
- .bind(CastBindName);
+ implicitCastExpr(hasSourceExpression(IntTypeExpr)).bind(CastBindName);
+
+ const auto ExplicitCastExpr =
+ anyOf(explicitCastExpr(has(ImplicitCastExpr)),
+ ignoringImpCasts(explicitCastExpr(has(ImplicitCastExpr))));
- const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
- const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
- const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
+ // Match function calls not directly wrapped by an implicit cast
+ const auto CallIntExpr = callExpr(HasIntegerType).bind(CastBindName);
- return expr(anyOf(ImplicitCastExpr, CStyleCastExpr, StaticCastExpr,
- FunctionalCastExpr));
+ return expr(anyOf(ImplicitCastExpr, ExplicitCastExpr, CallIntExpr));
}
static StringRef parseOpCode(BinaryOperator::Opcode Code) {
@@ -91,7 +93,8 @@ void UseIntegerSignComparisonCheck::storeOptions(
void UseIntegerSignComparisonCheck::registerMatchers(MatchFinder *Finder) {
const auto SignedIntCastExpr = intCastExpression(true, "sIntCastExpression");
- const auto UnSignedIntCastExpr = intCastExpression(false);
+ const auto UnSignedIntCastExpr =
+ intCastExpression(false, "uIntCastExpression");
// Flag all operators "==", "<=", ">=", "<", ">", "!="
// that are used between signed/unsigned
@@ -112,14 +115,17 @@ void UseIntegerSignComparisonCheck::registerPPCallbacks(
void UseIntegerSignComparisonCheck::check(
const MatchFinder::MatchResult &Result) {
const auto *SignedCastExpression =
- Result.Nodes.getNodeAs<ImplicitCastExpr>("sIntCastExpression");
- assert(SignedCastExpression);
+ Result.Nodes.getNodeAs<CastExpr>("sIntCastExpression");
+ const auto *UnsignedCastExpression =
+ Result.Nodes.getNodeAs<CastExpr>("uIntCastExpression");
+ const auto *CastExpression =
+ SignedCastExpression ? SignedCastExpression : UnsignedCastExpression;
+ assert(CastExpression);
// Ignore the match if we know that the signed int value is not negative.
Expr::EvalResult EVResult;
- if (!SignedCastExpression->isValueDependent() &&
- SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
- *Result.Context)) {
+ if (!CastExpression->isValueDependent() &&
+ CastExpression->getSubExpr()->EvaluateAsInt(EVResult, *Result.Context)) {
const llvm::APSInt SValue = EVResult.Val.getInt();
if (SValue.isNonNegative())
return;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 19ccd1790e757..882ee0015df17 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -237,6 +237,10 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-designated-initializers>` check by avoiding
diagnosing designated initializers for ``std::array`` initializations.
+- Improved :doc:`modernize-use-integer-sign-comparison
+ <clang-tidy/checks/modernize/use-integer-sign-comparison>` check by matching
+ valid integer expressions not directly wrapped around an implicit cast.
+
- Improved :doc:`modernize-use-ranges
<clang-tidy/checks/modernize/use-ranges>` check by updating suppress
warnings logic for ``nullptr`` in ``std::find``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
index e0a84ef5aed26..1af24c28e048d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
@@ -121,3 +121,100 @@ int AllComparisons() {
return 0;
}
+
+namespace PR127471 {
+ int getSignedValue();
+ unsigned int getUnsignedValue();
+
+ void callExprTest() {
+
+ if (getSignedValue() < getUnsignedValue())
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_less(getSignedValue() , getUnsignedValue()))
+
+ int sVar = 0;
+ if (getUnsignedValue() > sVar)
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_greater(getUnsignedValue() , sVar))
+
+ unsigned int uVar = 0;
+ if (getSignedValue() > uVar)
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_greater(getSignedValue() , uVar))
+
+ }
+
+ // Add a class with member functions for testing member function calls
+ class TestClass {
+ public:
+ int getSignedValue() { return -5; }
+ unsigned int getUnsignedValue() { return 5; }
+ };
+
+ void memberFunctionTests() {
+ TestClass obj;
+
+ if (obj.getSignedValue() < obj.getUnsignedValue())
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_less(obj.getSignedValue() , obj.getUnsignedValue()))
+ }
+
+ void castFunctionTests() {
+ // C-style casts with function calls
+ if ((int)getUnsignedValue() < (unsigned int)getSignedValue())
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_less(getUnsignedValue(),getSignedValue()))
+
+
+ // Static casts with function calls
+ if (static_cast<int>(getUnsignedValue()) < static_cast<unsigned int>(getSignedValue()))
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_less(getUnsignedValue(),getSignedValue()))
+ }
+
+ // Define tests
+ #define SIGNED_FUNC getSignedValue()
+ #define UNSIGNED_FUNC getUnsignedValue()
+
+ void defineTests() {
+ if (SIGNED_FUNC < UNSIGNED_FUNC)
+ return;
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+// CHECK-FIXES: if (std::cmp_less(SIGNED_FUNC , UNSIGNED_FUNC))
+ }
+
+ // Template tests (should not warn)
+ template <typename T1>
+ void templateFunctionTest(T1 value) {
+ if (value() < getUnsignedValue())
+ return;
+
+ if (value() < (getSignedValue() || getUnsignedValue()))
+ return;
+ }
+
+//GH127471
+ struct A
+ {
+ unsigned size() const;
+ };
+
+ struct B
+ {
+ A a;
+
+ bool foo(const A& a2) const {
+ return (int)a2.size() == size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
+ // CHECK-FIXES: return std::cmp_equal(a2.size(), size())
+ }
+
+ int size() const { return (int)a.size(); }
+ };
+} // namespace PR127471
More information about the cfe-commits
mailing list