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

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;

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``.

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