https://github.com/divvelarakesh1 created https://github.com/llvm/llvm-project/pull/167148
None >From 7a78ba2775b1771128752329320c5679f3aea238 Mon Sep 17 00:00:00 2001 From: Divvela Rakesh <[email protected]> Date: Sat, 8 Nov 2025 20:04:36 +0530 Subject: [PATCH 1/2] "readability check to convert numerical constants to std::numeric_limits" --- .../readability/NumericlimitmaxcheckCheck.cpp | 192 ++++++++++++++++++ .../readability/NumericlimitmaxcheckCheck.h | 35 ++++ .../readability/NumericLimitMaxCheck.cpp | 101 +++++++++ 3 files changed, 328 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp 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 >From 278c0f82d735b8dc3928e2fa624426515b19e6e5 Mon Sep 17 00:00:00 2001 From: Divvela Rakesh <[email protected]> Date: Sat, 8 Nov 2025 21:10:50 +0530 Subject: [PATCH 2/2] "readability check to convert numerical constants to std::numeric_limits" --- .../readability/NumericlimitmaxcheckCheck.cpp | 56 +++++++------------ .../readability/NumericLimitMaxCheck.cpp | 4 +- 2 files changed, 22 insertions(+), 38 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp b/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp index fd68564fc0c23..7b16a8b82497d 100644 --- a/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp @@ -38,8 +38,6 @@ void NumericlimitmaxcheckCheck::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus) return; - // ... inside registerMatchers() ... - auto NegOneLiteral = integerLiteral(equals(-1)); auto ZeroLiteral = integerLiteral(equals(0)); @@ -73,21 +71,16 @@ void NumericlimitmaxcheckCheck::registerMatchers(MatchFinder *Finder) { 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. + // To handle ternary branch case 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())) + hasAncestor(conditionalOperator( + hasAncestor(implicitCastExpr(hasType(isUnsignedInteger() + )) .bind("outerCast"))))) .bind("unsignedMaxExpr"); - // *** MODIFY THIS PART *** - auto OldCombined = + auto Combined = expr(anyOf( ExplicitCastOfNegOrBitnot, ImplicitNegOneToUnsigned, @@ -95,26 +88,26 @@ void NumericlimitmaxcheckCheck::registerMatchers(MatchFinder *Finder) { UnsignedLiteralNegOne )).bind("unsignedMaxExpr"); - Finder->addMatcher(OldCombined, this); - Finder->addMatcher(TernaryBranch, this); // Add the new matcher + Finder->addMatcher(Combined, this); + Finder->addMatcher(TernaryBranch, this); } 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 + const auto *E = Result.Nodes.getNodeAs<Expr>("unsignedMaxExpr"); + const auto *OuterCast = Result.Nodes.getNodeAs<CastExpr>("outerCast"); if (!E) return; - ASTContext &Ctx = *Result.Context; // Get context *before* first use + ASTContext &Ctx = *Result.Context; // Get context before first use QualType QT; if (OuterCast) { - // This is our new ternary matcher. Get type from the *cast*. + // This is ternary matcher. Get type from the cast. QT = OuterCast->getType(); } else { - // This is the old logic. Get type from the bound expression. + // Get type from the bound expression. QT = E->getType(); } @@ -123,19 +116,14 @@ const auto *E = Result.Nodes.getNodeAs<Expr>("unsignedMaxExpr"); 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 + auto Parents = Ctx.getParents(*E); 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 + // Skip this match, avoids double reporting return; } } @@ -153,14 +141,10 @@ const auto *E = Result.Nodes.getNodeAs<Expr>("unsignedMaxExpr"); } } } - // ... (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(); @@ -171,22 +155,22 @@ const auto *E = Result.Nodes.getNodeAs<Expr>("unsignedMaxExpr"); Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()), SM, getLangOpts()); - // Build replacement text + // Suggestion to the user regarding the usage of unsigned ~0 or -1 std::string Replacement = "std::numeric_limits<" + TypeStr + ">::max()"; - // Create diagnostic + //gives warning message if unsigned ~0 or -1 are used instead of numeric_limit::max() auto Diag = diag(E->getBeginLoc(), "use 'std::numeric_limits<%0>::max()' instead of '%1'") << TypeStr << OriginalText; - // Add fix-it hints + //suggest hint for fixing it Diag << FixItHint::CreateReplacement(E->getSourceRange(), Replacement); - // Add include for <limits> + //includes the <limits> which contains the numeric_limits::max() 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 +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp index 06dce9422fc52..9230f40f6343b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp @@ -1,6 +1,6 @@ // 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); @@ -98,4 +98,4 @@ void test_no_warning() { // The check should ignore code from macros unsigned m = MY_MAX_MACRO; unsigned m2 = MY_MAX_MACRO_TILDE; -} \ No newline at end of file +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
