[clang-tools-extra] [clang-tidy] Create a check for signed and unsigned integers comparison (PR #113144)
Julian Schmidt via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 21 16:41:40 PST 2024
================
@@ -0,0 +1,182 @@
+//===--- UseIntegerSignComparisonCheck.cpp - clang-tidy -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseIntegerSignComparisonCheck.h"
+#include "clang/AST/Expr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
+
+namespace clang::tidy::modernize {
+static BindableMatcher<clang::Stmt>
+intCastExpression(bool IsSigned, const std::string &CastBindName) {
+ auto IntTypeExpr = expr(hasType(hasCanonicalType(qualType(
+ isInteger(), IsSigned ? isSignedInteger() : unless(isSignedInteger())))));
+
+ const auto ImplicitCastExpr =
+ implicitCastExpr(hasSourceExpression(IntTypeExpr)).bind(CastBindName);
+
+ const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
+ const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
+ const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
+
+ return expr(anyOf(ImplicitCastExpr, CStyleCastExpr, StaticCastExpr,
+ FunctionalCastExpr));
+}
+
+static StringRef parseOpCode(BinaryOperator::Opcode Code) {
+ switch (Code) {
+ case BO_LT:
+ return "cmp_less";
+ case BO_GT:
+ return "cmp_greater";
+ case BO_LE:
+ return "cmp_less_equal";
+ case BO_GE:
+ return "cmp_greater_equal";
+ case BO_EQ:
+ return "cmp_equal";
+ case BO_NE:
+ return "cmp_not_equal";
+ default:
+ return "";
+ }
+}
+
+UseIntegerSignComparisonCheck::UseIntegerSignComparisonCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+ utils::IncludeSorter::IS_LLVM),
+ areDiagsSelfContained()) {}
+
+void UseIntegerSignComparisonCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
+}
+
+void UseIntegerSignComparisonCheck::registerMatchers(MatchFinder *Finder) {
+ const auto SignedIntCastExpr = intCastExpression(true, "sIntCastExpression");
+ const auto UnSignedIntCastExpr =
+ intCastExpression(false, "uIntCastExpression");
+
+ // Flag all operators "==", "<=", ">=", "<", ">", "!="
+ // that are used between signed/unsigned
+ const auto CompareOperator =
+ binaryOperator(hasAnyOperatorName("==", "<=", ">=", "<", ">", "!="),
+ hasOperands(SignedIntCastExpr, UnSignedIntCastExpr),
+ unless(isInTemplateInstantiation()))
+ .bind("intComparison");
+
+ Finder->addMatcher(CompareOperator, this);
+}
+
+void UseIntegerSignComparisonCheck::registerPPCallbacks(
+ const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+ IncludeInserter.registerPreprocessor(PP);
+}
+
+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;
+ if (!SignedCastExpression->isValueDependent() &&
+ SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
+ *Result.Context)) {
+ const llvm::APSInt SValue = EVResult.Val.getInt();
+ if (SValue.isNonNegative())
+ return;
+ }
+
+ const auto *BinaryOp =
+ Result.Nodes.getNodeAs<BinaryOperator>("intComparison");
+ if (BinaryOp == nullptr)
+ return;
+
+ const BinaryOperator::Opcode OpCode = BinaryOp->getOpcode();
+ const Expr *LHS = BinaryOp->getLHS()->IgnoreParenImpCasts();
+ const Expr *RHS = BinaryOp->getRHS()->IgnoreParenImpCasts();
+ if (LHS == nullptr || RHS == nullptr)
+ return;
+
+ const Expr *SubExprLHS = nullptr;
+ const Expr *SubExprRHS = nullptr;
+ SourceRange R1 = SourceRange(LHS->getBeginLoc());
+ SourceRange R2 = SourceRange(BinaryOp->getOperatorLoc());
+
+ if (const auto *LHSCast = BinaryOp->getLHS() == SignedCastExpression
+ ? SignedCastExpression
+ : UnSignedCastExpression) {
+ if (LHSCast->isPartOfExplicitCast()) {
+ SubExprLHS = LHSCast->getSubExpr();
+ R1 = SourceRange(LHS->getBeginLoc(), SubExprLHS->getBeginLoc());
+ }
+ }
+
+ if (const auto *RHSCast = BinaryOp->getLHS() == SignedCastExpression
+ ? UnSignedCastExpression
+ : SignedCastExpression) {
+ if (RHSCast->isPartOfExplicitCast()) {
+ SubExprRHS = RHSCast->getSubExpr();
+ R2.setEnd(SubExprRHS->getBeginLoc());
+ }
+ }
+
+ DiagnosticBuilder Diag =
+ diag(BinaryOp->getBeginLoc(),
+ "comparison between 'signed' and 'unsigned' integers");
+
+ if (!getLangOpts().CPlusPlus20)
+ return;
+
+ const std::string CmpNamespace =
+ llvm::Twine("std::" + parseOpCode(OpCode)).str();
+ const std::string CmpHeader = "<utility>";
+
+ // Prefer modernize-use-integer-sign-comparison when C++20 is available!
+ if (SubExprLHS) {
+ StringRef ExplicitLhsString =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(
+ SubExprLHS->IgnoreCasts()->getSourceRange()),
+ *Result.SourceManager, getLangOpts());
+ Diag << FixItHint::CreateReplacement(
+ R1, llvm::Twine(CmpNamespace + "(" + ExplicitLhsString).str());
+ } else {
+ Diag << FixItHint::CreateInsertion(R1.getBegin(),
+ llvm::Twine(CmpNamespace + "(").str());
+ }
+
+ StringRef ExplicitLhsRhsString =
+ SubExprRHS ? Lexer::getSourceText(
+ CharSourceRange::getTokenRange(
+ SubExprRHS->IgnoreCasts()->getSourceRange()),
+ *Result.SourceManager, getLangOpts())
+ : "";
+ Diag << FixItHint::CreateReplacement(
+ R2, llvm::Twine(", " + ExplicitLhsRhsString).str());
+ Diag << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(
+ Result.SourceManager->getSpellingLoc(RHS->getEndLoc()), 0,
+ *Result.SourceManager, Result.Context->getLangOpts()),
+ ")");
----------------
5chmidti wrote:
w.r.t. my previous comment, when using the attached changes, no call to `getSourceText` is used, which in turn means, that no macros are expanded. IMO, it is a very common pattern to compare a value to the value of a macro, and expanding that macro could break the user's code.
```suggestion
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 = SourceRange(LHS->getBeginLoc());
SourceRange R2 = SourceRange(BinaryOp->getOperatorLoc());
SourceRange R3 = SourceRange(Lexer::getLocForEndOfToken(
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));
R2.setBegin(Lexer::getLocForEndOfToken(
SubExprLHS->getEndLoc(), 0, *Result.SourceManager, getLangOpts()));
}
if (const auto *RHSCast = llvm::dyn_cast<ExplicitCastExpr>(RHS)) {
SubExprRHS = RHSCast->getSubExpr();
R2.setEnd(SubExprRHS->getBeginLoc().getLocWithOffset(-1));
}
DiagnosticBuilder Diag =
diag(BinaryOp->getBeginLoc(),
"comparison between 'signed' and 'unsigned' integers");
if (!getLangOpts().CPlusPlus20)
return;
const std::string CmpNamespace = ("std::" + parseOpCode(OpCode)).str();
const std::string CmpHeader = "<utility>";
// Prefer modernize-use-integer-sign-comparison when C++20 is available!
Diag << FixItHint::CreateReplacement(
CharSourceRange(R1, SubExprLHS != nullptr),
llvm::Twine(CmpNamespace + "(").str());
Diag << FixItHint::CreateReplacement(R2, ",");
Diag << FixItHint::CreateReplacement(CharSourceRange::getCharRange(R3), ")");
```
passes the tests (needs whitespace adjustments though), and also
```c++
if (static_cast<unsigned int>(uArray[2]) < static_cast<int>(sArray[2]))
return 0;
// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
// CHECK-FIXES: if (std::cmp_less(uArray[2],sArray[2]))
if ((unsigned int)uArray[3] < (int)sArray[3])
return 0;
// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
// CHECK-FIXES: if (std::cmp_less(uArray[3],sArray[3]))
if ((unsigned int)(uArray[4]) < (int)(sArray[4]))
return 0;
// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
// CHECK-FIXES: if (std::cmp_less((uArray[4]),(sArray[4])))
if (uArray[5] > sArray[5])
return 0;
// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: comparison between 'signed' and 'unsigned' integers [modernize-use-integer-sign-comparison]
// CHECK-FIXES: if (std::cmp_greater(uArray[5] , sArray[5]))
#define VALUE sArray[6]
if (uArray[6] > VALUE)
return 0;
// 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))
```
https://github.com/llvm/llvm-project/pull/113144
More information about the cfe-commits
mailing list