"readability check to convert numerical constants to std::numeric_limits"

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

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

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;
+}