llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Divvela Rakesh (divvelarakesh1) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/167145.diff 3 Files Affected: - (added) clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp (+192) - (added) clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.h (+35) - (added) clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp (+101) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp b/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp new file mode 100644 index 0000000000000..fd68564fc0c23 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp @@ -0,0 +1,192 @@ +//===----------------------------------------------------------------------===// +// +// 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 "NumericlimitmaxcheckCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/SourceManager.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +NumericlimitmaxcheckCheck::NumericlimitmaxcheckCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + Inserter(Options.getLocalOrGlobal("IncludeStyle", utils::IncludeSorter::IS_LLVM), + areDiagsSelfContained()) {} + +void NumericlimitmaxcheckCheck::registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + Inserter.registerPreprocessor(PP); +} + +void NumericlimitmaxcheckCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", Inserter.getStyle()); +} + +bool NumericlimitmaxcheckCheck::isLanguageVersionSupported(const LangOptions &LangOpts) const { + return LangOpts.CPlusPlus; +} + +void NumericlimitmaxcheckCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + // ... inside registerMatchers() ... + + auto NegOneLiteral = integerLiteral(equals(-1)); + auto ZeroLiteral = integerLiteral(equals(0)); + + auto NegOneExpr = anyOf( + NegOneLiteral, + unaryOperator(hasOperatorName("-"), + hasUnaryOperand(integerLiteral(equals(1))))); + + auto BitNotZero = unaryOperator(hasOperatorName("~"), + hasUnaryOperand(ZeroLiteral)); + + // Match implicit cast of -1 to unsigned + auto ImplicitNegOneToUnsigned = + implicitCastExpr( + hasSourceExpression(ignoringParenImpCasts(anyOf(NegOneExpr, BitNotZero))), + hasType(isUnsignedInteger())); + + // Match explicit cast to unsigned of either -1 or ~0 + auto ExplicitCastOfNegOrBitnot = + explicitCastExpr( + hasDestinationType(isUnsignedInteger()), + hasSourceExpression(ignoringParenImpCasts(anyOf(NegOneExpr, BitNotZero)))); + + // Match ~0 with unsigned type + auto UnsignedBitNotZero = + unaryOperator( + hasOperatorName("~"), + hasUnaryOperand(ZeroLiteral), + hasType(isUnsignedInteger())); + + auto UnsignedLiteralNegOne = + integerLiteral(equals(-1), hasType(isUnsignedInteger())); + + // *** ADD THIS NEW MATCHER *** + // Matches -1 or ~0 when they are a branch of a ternary operator + // that is itself being implicitly cast to unsigned. + auto TernaryBranch = + expr(anyOf(NegOneExpr, BitNotZero), + hasAncestor( // <-- Use hasAncestor to look up the tree + conditionalOperator( + // Check that the conditional operator itself has an ancestor + // which is the implicit cast to unsigned + hasAncestor(implicitCastExpr(hasType(isUnsignedInteger())) + .bind("outerCast"))))) + .bind("unsignedMaxExpr"); + + // *** MODIFY THIS PART *** + auto OldCombined = + expr(anyOf( + ExplicitCastOfNegOrBitnot, + ImplicitNegOneToUnsigned, + UnsignedBitNotZero, + UnsignedLiteralNegOne + )).bind("unsignedMaxExpr"); + + Finder->addMatcher(OldCombined, this); + Finder->addMatcher(TernaryBranch, this); // Add the new matcher +} + + +void NumericlimitmaxcheckCheck::check(const MatchFinder::MatchResult &Result) { +const auto *E = Result.Nodes.getNodeAs<Expr>("unsignedMaxExpr"); + const auto *OuterCast = Result.Nodes.getNodeAs<CastExpr>("outerCast"); // Get the cast + + if (!E) + return; + + ASTContext &Ctx = *Result.Context; // Get context *before* first use + + QualType QT; + if (OuterCast) { + // This is our new ternary matcher. Get type from the *cast*. + QT = OuterCast->getType(); + } else { + // This is the old logic. Get type from the bound expression. + QT = E->getType(); + } + + if (E->getBeginLoc().isInvalid() || E->getBeginLoc().isMacroID()) + return; + + const SourceManager &SM = Ctx.getSourceManager(); + + // *** ADD THIS LOGIC BLOCK *** + // This logic prevents double-reporting for the *old* matchers. + // We skip it for the new ternary matcher (when OuterCast is not null) + // because the ternary matcher binds the *inner* expression, and we + // *do* want to report it. + if (!OuterCast) { + auto Parents = Ctx.getParents(*E); // This fixes the [-Wunused] warning + if (!Parents.empty()) { + for (const auto &Parent : Parents) { + // Check if parent is an explicit cast to unsigned + if (const auto *ParentCast = Parent.get<ExplicitCastExpr>()) { + if (ParentCast->getType()->isUnsignedIntegerType()) { + // Skip this match - the cast itself will be (or was) reported + return; + } + } + // Also check if parent is an implicit cast that's part of an explicit cast chain + if (const auto *ImplicitCast = Parent.get<ImplicitCastExpr>()) { + auto GrandParents = Ctx.getParents(*ImplicitCast); + for (const auto &GP : GrandParents) { + if (const auto *GPCast = GP.get<ExplicitCastExpr>()) { + if (GPCast->getType()->isUnsignedIntegerType()) { + return; + } + } + } + } + } + } + } + // ... (rest of parent checking logic) ... + // ... + // Determine the unsigned destination type + // QualType QT = E->getType(); // This line is moved up and modified + if (QT.isNull() || !QT->isUnsignedIntegerType()) + return; + + // Get a printable type string + std::string TypeStr = QT.getUnqualifiedType().getAsString(); + if (const auto *Typedef = QT->getAs<TypedefType>()) { + TypeStr = Typedef->getDecl()->getName().str(); + } + + // Get original source text for diagnostic message + StringRef OriginalText = + Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()), + SM, getLangOpts()); + + // Build replacement text + std::string Replacement = "std::numeric_limits<" + TypeStr + ">::max()"; + + // Create diagnostic + auto Diag = diag(E->getBeginLoc(), + "use 'std::numeric_limits<%0>::max()' instead of '%1'") + << TypeStr << OriginalText; + + // Add fix-it hints + Diag << FixItHint::CreateReplacement(E->getSourceRange(), Replacement); + + // Add include for <limits> + FileID FID = SM.getFileID(E->getBeginLoc()); + if (auto IncludeHint = Inserter.createIncludeInsertion(FID, "<limits>")) { + Diag << *IncludeHint; + } +} + +} // namespace clang::tidy::readability \ No newline at end of file diff --git a/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.h b/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.h new file mode 100644 index 0000000000000..1a3ed29e7d5fa --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.h @@ -0,0 +1,35 @@ +//===----------------------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NUMERICLIMITMAXCHECKCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NUMERICLIMITMAXCHECKCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/IncludeInserter.h" + +namespace clang::tidy::readability { + +/// Suggest replacement of values like -1 / `~0` (when used as unsigned) +/// with std::numeric_limits<T>::max() for unsigned integer types. +class NumericlimitmaxcheckCheck : public ClangTidyCheck { +public: + NumericlimitmaxcheckCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; + + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpander) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + utils::IncludeInserter Inserter; +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NUMERICLIMITMAXCHECKCHECK_H \ No newline at end of file diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp new file mode 100644 index 0000000000000..06dce9422fc52 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp @@ -0,0 +1,101 @@ +// RUN: %check_clang_tidy %s readability-NumericLimitMaxCheck %t + +// Defines a typedef, which the check should respect and use in its fix. +typedef unsigned long long my_uint_t; + +void test_arg(unsigned int); + +void test_unsigned_literals() { + + unsigned int a = -1; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned int a = std::numeric_limits<unsigned int>::max(); + + unsigned int b = ~0; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '~0' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned int b = std::numeric_limits<unsigned int>::max(); + + unsigned long c = (unsigned long)(-1); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'std::numeric_limits<unsigned long>::max()' instead of '(unsigned long)(-1)' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned long c = std::numeric_limits<unsigned long>::max(); + + unsigned long d = (unsigned long)(~0); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'std::numeric_limits<unsigned long>::max()' instead of '(unsigned long)(~0)' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned long d = std::numeric_limits<unsigned long>::max(); + +} + +void test_comparisons(unsigned x) { + if (x == -1) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: if (x == std::numeric_limits<unsigned int>::max()) { + ; + } + + if (x == (unsigned)(~0)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '(unsigned)(~0)' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: if (x == std::numeric_limits<unsigned int>::max()); + +} + +void test_cpp_casts_and_other_types() { + unsigned int e = static_cast<unsigned int>(-1); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of 'static_cast<unsigned int>(-1)' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned int e = std::numeric_limits<unsigned int>::max(); + + unsigned int f = unsigned(-1); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of 'unsigned(-1)' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned int f = std::numeric_limits<unsigned int>::max(); + + unsigned short s = -1; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use 'std::numeric_limits<unsigned short>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned short s = std::numeric_limits<unsigned short>::max(); + + unsigned char c = ~0; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'std::numeric_limits<unsigned char>::max()' instead of '~0' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned char c = std::numeric_limits<unsigned char>::max(); + + // Tests that the check correctly uses the typedef'd name "my_uint_t" + my_uint_t u = -1; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use 'std::numeric_limits<my_uint_t>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: my_uint_t u = std::numeric_limits<my_uint_t>::max(); +} + +unsigned test_other_contexts(bool x) { + // Test function arguments + test_arg(-1); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: test_arg(std::numeric_limits<unsigned int>::max()); + + // Test ternary operator + unsigned k = x ? -1 : 0; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned k = x ? std::numeric_limits<unsigned int>::max() : 0; + + unsigned k2 = x ? 0 : ~0; + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '~0' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: unsigned k2 = x ? 0 : std::numeric_limits<unsigned int>::max(); + + // Test return values + return -1; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck] + // CHECK-FIXES: return std::numeric_limits<unsigned int>::max(); +} + + +#define MY_MAX_MACRO -1 +#define MY_MAX_MACRO_TILDE ~0 + +void test_no_warning() { + int a = -1; + unsigned u = 0; + int b = ~0; + unsigned c = 42; + + // This is (max - 1), not max + unsigned d = (unsigned)-2; + + // The check should ignore code from macros + unsigned m = MY_MAX_MACRO; + unsigned m2 = MY_MAX_MACRO_TILDE; +} \ No newline at end of file `````````` </details> https://github.com/llvm/llvm-project/pull/167145 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
