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 <davidriv...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
