[clang-tidy][NFC] Clean up and slightly optimize modernize-use-integer-sign-comparison

[localspook]

No description provided.

[llvmbot]

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

@llvm/pr-subscribers-clang-tidy

Author: Victor Chernyakin (localspook)

Changes

---

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

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp (+13-22)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp index 0003429c62890..a3d3f0fa0ec93 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp @@ -34,13 +34,12 @@ AST_MATCHER(clang::QualType, isActualChar) { } // namespace static BindableMatcher<clang::Stmt> -intCastExpression(bool IsSigned, - const std::string &CastBindName = std::string()) { +intCastExpression(bool IsSigned, StringRef CastBindName = {}) { // 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( - isInteger(), IsSigned ? isSignedInteger() : isUnsignedInteger(), + IsSigned ? isSignedInteger() : isUnsignedInteger(), unless(isActualChar()), unless(booleanType()), unless(enumType()))))); const auto ImplicitCastExpr = @@ -71,7 +70,7 @@ static StringRef parseOpCode(BinaryOperator::Opcode Code) { case BO_NE: return "cmp_not_equal"; default: - return ""; + llvm_unreachable("invalid opcode"); } } @@ -119,23 +118,16 @@ void UseIntegerSignComparisonCheck::check( Expr::EvalResult EVResult; if (!SignedCastExpression->isValueDependent() && SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult, - *Result.Context)) { - const llvm::APSInt SValue = EVResult.Val.getInt(); - if (SValue.isNonNegative()) - return; - } + *Result.Context) && + EVResult.Val.getInt().isNonNegative()) + return; const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("intComparison"); - if (BinaryOp == nullptr) - return; - - const BinaryOperator::Opcode OpCode = BinaryOp->getOpcode(); + assert(BinaryOp); const Expr *LHS = BinaryOp->getLHS()->IgnoreImpCasts(); const Expr *RHS = BinaryOp->getRHS()->IgnoreImpCasts(); - if (LHS == nullptr || RHS == nullptr) - return; const Expr *SubExprLHS = nullptr; const Expr *SubExprRHS = nullptr; SourceRange R1(LHS->getBeginLoc()); @@ -144,8 +136,7 @@ void UseIntegerSignComparisonCheck::check( RHS->getEndLoc(), 0, *Result.SourceManager, getLangOpts())); if (const auto *LHSCast = llvm::dyn_cast<ExplicitCastExpr>(LHS)) { SubExprLHS = LHSCast->getSubExpr(); - R1 = SourceRange(LHS->getBeginLoc(), - SubExprLHS->getBeginLoc().getLocWithOffset(-1)); + R1.setEnd(SubExprLHS->getBeginLoc().getLocWithOffset(-1)); R2.setBegin(Lexer::getLocForEndOfToken( SubExprLHS->getEndLoc(), 0, *Result.SourceManager, getLangOpts())); } @@ -156,21 +147,21 @@ void UseIntegerSignComparisonCheck::check( DiagnosticBuilder Diag = diag(BinaryOp->getBeginLoc(), "comparison between 'signed' and 'unsigned' integers"); - std::string CmpNamespace; - llvm::StringRef CmpHeader; + StringRef CmpNamespace; + StringRef CmpHeader; if (getLangOpts().CPlusPlus20) { CmpHeader = "<utility>"; - CmpNamespace = llvm::Twine("std::" + parseOpCode(OpCode)).str(); + CmpNamespace = "std::"; } else if (getLangOpts().CPlusPlus17 && EnableQtSupport) { CmpHeader = "<QtCore/q20utility.h>"; - CmpNamespace = llvm::Twine("q20::" + parseOpCode(OpCode)).str(); + CmpNamespace = "q20::"; } // Prefer modernize-use-integer-sign-comparison when C++20 is available! Diag << FixItHint::CreateReplacement( CharSourceRange(R1, SubExprLHS != nullptr), - llvm::Twine(CmpNamespace + "(").str()); + Twine(CmpNamespace + parseOpCode(BinaryOp->getOpcode()) + "(").str()); Diag << FixItHint::CreateReplacement(R2, ","); Diag << FixItHint::CreateReplacement(CharSourceRange::getCharRange(R3), ")"); 

[localspook]

isInteger() is redundant when we have isSignedInteger() or isUnsignedInteger()

[localspook]

getInt() returns a reference, so this is an unnecessary copy

[localspook]

I don't believe either of BinaryOperation's subexpressions can be null

[vbvictor]

But can IgnoreImpCasts return null? (Genuine question, I'll look into sources a bit later)

[localspook]

I would be surprised if it did; in my understanding, all it does is optionally skip past some AST nodes, so what would cause it to return null? (It would be good to check though, yep)

[vbvictor]

I guess should not

[localspook]

IgnoreImpCasts repeatedly applies this function to the Expr it's called on:

llvm-project/clang/include/clang/AST/IgnoreExpr.h

Lines 48 to 56 in 4d6f43d

	inline Expr *IgnoreImplicitCastsSingleStep(Expr *E) {
	if (auto *ICE = dyn_cast<ImplicitCastExpr>(E))
	return ICE->getSubExpr();
	if (auto *FE = dyn_cast<FullExpr>(E))
	return FE->getSubExpr();
	return E;
	}

until it reaches a fixed point. I don't think ImplicitCastExpr or FullExpr can have a null subexpression, so I think this change is safe

[localspook]

StringRef is enough here, we don't need std::string

[localspook]

At this point, R1's begin location is already equal to LHS->getBeginLoc(), so we only need to update the end location

[localspook]

Moving the parseOpCode() call down here removes the duplication above

[llvm-ci]

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-sie-ubuntu-fast running on sie-linux-worker while building clang-tools-extra at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/38185

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure) ******************** TEST 'lld :: ELF/gc-sections-strip-debug.s' FAILED ******************** Exit Code: 250 Command Output (stdout): -- # RUN: at line 5 /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/llvm-mc -filetype=obj -triple=x86_64 /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/lld/test/ELF/gc-sections-strip-debug.s -o /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/lld/test/ELF/Output/gc-sections-strip-debug.s.tmp.o # executed command: /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/llvm-mc -filetype=obj -triple=x86_64 /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/lld/test/ELF/gc-sections-strip-debug.s -o /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/lld/test/ELF/Output/gc-sections-strip-debug.s.tmp.o # note: command had no output on stdout or stderr # RUN: at line 6 /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/ld.lld /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/lld/test/ELF/Output/gc-sections-strip-debug.s.tmp.o --gc-sections --strip-debug -o /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/lld/test/ELF/Output/gc-sections-strip-debug.s.tmp # executed command: /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/ld.lld /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/lld/test/ELF/Output/gc-sections-strip-debug.s.tmp.o --gc-sections --strip-debug -o /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/lld/test/ELF/Output/gc-sections-strip-debug.s.tmp # .---command stderr------------ # | terminate called after throwing an instance of 'std::system_error' # | what(): Resource temporarily unavailable # `----------------------------- # error: command failed with exit status: 250 -- ********************