[clang-tidy] Fix modernize-use-integer-sign-comparison comparison

[qt-tatiana]

• modernize-use-integer-sign-comparison should ignore a comparison between the signed wide type and the unsigned narrow type, see [clang-tidy] Ignore modernize-use-integer-sign-comparison between signed wide type and unsigned narrow type #120867

[llvmbot]

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

@llvm/pr-subscribers-clang-tidy

Author: None (qt-tatiana)

Changes

• modernize-use-integer-sign-comparison should ignore a comparison between the signed wide type and the unsigned narrow type, see #120867

---

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

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp (+13-1)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp (+22-2)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp index 8f807bc0a96d56..ebcfafcd391145 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp @@ -89,7 +89,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 @@ -111,7 +112,10 @@ void UseIntegerSignComparisonCheck::check( const MatchFinder::MatchResult &Result) { const auto *SignedCastExpression = Result.Nodes.getNodeAs<ImplicitCastExpr>("sIntCastExpression"); + const auto *UnSignedCastExpression = + Result.Nodes.getNodeAs<ImplicitCastExpr>("uIntCastExpression"); assert(SignedCastExpression); + assert(UnSignedCastExpression); // Ignore the match if we know that the signed int value is not negative. Expr::EvalResult EVResult; @@ -134,6 +138,13 @@ void UseIntegerSignComparisonCheck::check( const Expr *RHS = BinaryOp->getRHS()->IgnoreImpCasts(); if (LHS == nullptr || RHS == nullptr) return; + + if (Result.Context->getTypeSize( + SignedCastExpression->getSubExpr()->getType()) > + Result.Context->getTypeSize( + UnSignedCastExpression->getSubExpr()->getType())) + return; + const Expr *SubExprLHS = nullptr; const Expr *SubExprRHS = nullptr; SourceRange R1 = SourceRange(LHS->getBeginLoc()); @@ -151,6 +162,7 @@ void UseIntegerSignComparisonCheck::check( SubExprRHS = RHSCast->getSubExpr(); R2.setEnd(SubExprRHS->getBeginLoc().getLocWithOffset(-1)); } + DiagnosticBuilder Diag = diag(BinaryOp->getBeginLoc(), "comparison between 'signed' and 'unsigned' integers"); 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 99f00444c2d3f3..85eda925d5903c 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 @@ -32,7 +32,7 @@ int TemplateFuncParameters(T1 val1, T2 val2) { int AllComparisons() { unsigned int uVar = 42; - unsigned short uArray[7] = {0, 1, 2, 3, 9, 7, 9}; + unsigned int uArray[7] = {0, 1, 2, 3, 9, 7, 9}; int sVar = -42; short sArray[7] = {-1, -2, -8, -94, -5, -4, -6}; @@ -113,10 +113,30 @@ int AllComparisons() { // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison] // CHECK-FIXES: if (std::cmp_greater(uArray[6] , VALUE)) - FuncParameters(uVar); TemplateFuncParameter(sVar); TemplateFuncParameters(uVar, sVar); return 0; } + +bool foo1(int x, unsigned char y) { + return x == y; +// CHECK-MESSAGES-NOT: warning: +} + +bool foo2(int x, unsigned short y) { + return x == y; +// CHECK-MESSAGES-NOT: warning: +} + +bool bar1(unsigned int x, char y) { + return x == y; +// CHECK-MESSAGES-NOT: warning: +} + +bool bar2(unsigned int x, short y) { + return x == y; +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison] +// CHECK-FIXES: std::cmp_equal(x , y); +} 

[PiotrZSL]

Missing:

• Documentation entry to tell user about this behavior (single sentence)

To be considered:

• Configuration option to enable/disable this behavior

[HerrCai0907]

Please update release note.
I agree to enable this feature by default if adding an option.

[qt-tatiana]

Missing:

• Documentation entry to tell user about this behavior (single sentence)

To be considered:

• Configuration option to enable/disable this behavior

done

[qt-tatiana]

Please update release note. I agree to enable this feature by default if adding an option.

Done

[PiotrZSL]

Few small nits, but functionally ok.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[qt-tatiana]

Ping :)