[clang-tidy] Improve integer comparison by matching valid expressions outside implicitCastExpr for use-integer-sign-comparison

[RiverDave]

Addresses: #127471
Originally merged under: #134188 but reverted due to #143927

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)

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):

Notice how the signed operand is not wrapped by a cast in any way.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: David Rivera (RiverDave)

Changes

Expands check based on: #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)

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/144240.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp (+22-16)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp (+97)

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