Skip to content

Commit 7a78ba2

Browse files
divvelarakesh1divvelarakesh1
committed
"readability check to convert numerical constants to std::numeric_limits"
1 parent a0e222f commit 7a78ba2

File tree

3 files changed

+328
-0
lines changed

3 files changed

+328
-0
lines changed

‎clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.cpp‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "NumericlimitmaxcheckCheck.h"
10+
#include "clang/AST/ASTContext.h"
11+
#include "clang/AST/Expr.h"
12+
#include "clang/ASTMatchers/ASTMatchFinder.h"
13+
#include "clang/Basic/SourceManager.h"
14+
#include "clang/Lex/Lexer.h"
15+
16+
using namespace clang::ast_matchers;
17+
18+
namespace clang::tidy::readability {
19+
20+
NumericlimitmaxcheckCheck::NumericlimitmaxcheckCheck(StringRef Name, ClangTidyContext *Context)
21+
: ClangTidyCheck(Name, Context),
22+
Inserter(Options.getLocalOrGlobal("IncludeStyle", utils::IncludeSorter::IS_LLVM),
23+
areDiagsSelfContained()) {}
24+
25+
void NumericlimitmaxcheckCheck::registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
26+
Inserter.registerPreprocessor(PP);
27+
}
28+
29+
void NumericlimitmaxcheckCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
30+
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
31+
}
32+
33+
bool NumericlimitmaxcheckCheck::isLanguageVersionSupported(const LangOptions &LangOpts) const {
34+
return LangOpts.CPlusPlus;
35+
}
36+
37+
void NumericlimitmaxcheckCheck::registerMatchers(MatchFinder *Finder) {
38+
if (!getLangOpts().CPlusPlus)
39+
return;
40+
41+
// ... inside registerMatchers() ...
42+
43+
auto NegOneLiteral = integerLiteral(equals(-1));
44+
auto ZeroLiteral = integerLiteral(equals(0));
45+
46+
auto NegOneExpr = anyOf(
47+
NegOneLiteral,
48+
unaryOperator(hasOperatorName("-"),
49+
hasUnaryOperand(integerLiteral(equals(1)))));
50+
51+
auto BitNotZero = unaryOperator(hasOperatorName("~"),
52+
hasUnaryOperand(ZeroLiteral));
53+
54+
// Match implicit cast of -1 to unsigned
55+
auto ImplicitNegOneToUnsigned =
56+
implicitCastExpr(
57+
hasSourceExpression(ignoringParenImpCasts(anyOf(NegOneExpr, BitNotZero))),
58+
hasType(isUnsignedInteger()));
59+
60+
// Match explicit cast to unsigned of either -1 or ~0
61+
auto ExplicitCastOfNegOrBitnot =
62+
explicitCastExpr(
63+
hasDestinationType(isUnsignedInteger()),
64+
hasSourceExpression(ignoringParenImpCasts(anyOf(NegOneExpr, BitNotZero))));
65+
66+
// Match ~0 with unsigned type
67+
auto UnsignedBitNotZero =
68+
unaryOperator(
69+
hasOperatorName("~"),
70+
hasUnaryOperand(ZeroLiteral),
71+
hasType(isUnsignedInteger()));
72+
73+
auto UnsignedLiteralNegOne =
74+
integerLiteral(equals(-1), hasType(isUnsignedInteger()));
75+
76+
// *** ADD THIS NEW MATCHER ***
77+
// Matches -1 or ~0 when they are a branch of a ternary operator
78+
// that is itself being implicitly cast to unsigned.
79+
auto TernaryBranch =
80+
expr(anyOf(NegOneExpr, BitNotZero),
81+
hasAncestor( // <-- Use hasAncestor to look up the tree
82+
conditionalOperator(
83+
// Check that the conditional operator itself has an ancestor
84+
// which is the implicit cast to unsigned
85+
hasAncestor(implicitCastExpr(hasType(isUnsignedInteger()))
86+
.bind("outerCast")))))
87+
.bind("unsignedMaxExpr");
88+
89+
// *** MODIFY THIS PART ***
90+
auto OldCombined =
91+
expr(anyOf(
92+
ExplicitCastOfNegOrBitnot,
93+
ImplicitNegOneToUnsigned,
94+
UnsignedBitNotZero,
95+
UnsignedLiteralNegOne
96+
)).bind("unsignedMaxExpr");
97+
98+
Finder->addMatcher(OldCombined, this);
99+
Finder->addMatcher(TernaryBranch, this); // Add the new matcher
100+
}
101+
102+
103+
void NumericlimitmaxcheckCheck::check(const MatchFinder::MatchResult &Result) {
104+
const auto *E = Result.Nodes.getNodeAs<Expr>("unsignedMaxExpr");
105+
const auto *OuterCast = Result.Nodes.getNodeAs<CastExpr>("outerCast"); // Get the cast
106+
107+
if (!E)
108+
return;
109+
110+
ASTContext &Ctx = *Result.Context; // Get context *before* first use
111+
112+
QualType QT;
113+
if (OuterCast) {
114+
// This is our new ternary matcher. Get type from the *cast*.
115+
QT = OuterCast->getType();
116+
} else {
117+
// This is the old logic. Get type from the bound expression.
118+
QT = E->getType();
119+
}
120+
121+
if (E->getBeginLoc().isInvalid() || E->getBeginLoc().isMacroID())
122+
return;
123+
124+
const SourceManager &SM = Ctx.getSourceManager();
125+
126+
// *** ADD THIS LOGIC BLOCK ***
127+
// This logic prevents double-reporting for the *old* matchers.
128+
// We skip it for the new ternary matcher (when OuterCast is not null)
129+
// because the ternary matcher binds the *inner* expression, and we
130+
// *do* want to report it.
131+
if (!OuterCast) {
132+
auto Parents = Ctx.getParents(*E); // This fixes the [-Wunused] warning
133+
if (!Parents.empty()) {
134+
for (const auto &Parent : Parents) {
135+
// Check if parent is an explicit cast to unsigned
136+
if (const auto *ParentCast = Parent.get<ExplicitCastExpr>()) {
137+
if (ParentCast->getType()->isUnsignedIntegerType()) {
138+
// Skip this match - the cast itself will be (or was) reported
139+
return;
140+
}
141+
}
142+
// Also check if parent is an implicit cast that's part of an explicit cast chain
143+
if (const auto *ImplicitCast = Parent.get<ImplicitCastExpr>()) {
144+
auto GrandParents = Ctx.getParents(*ImplicitCast);
145+
for (const auto &GP : GrandParents) {
146+
if (const auto *GPCast = GP.get<ExplicitCastExpr>()) {
147+
if (GPCast->getType()->isUnsignedIntegerType()) {
148+
return;
149+
}
150+
}
151+
}
152+
}
153+
}
154+
}
155+
}
156+
// ... (rest of parent checking logic) ...
157+
// ...
158+
// Determine the unsigned destination type
159+
// QualType QT = E->getType(); // This line is moved up and modified
160+
if (QT.isNull() || !QT->isUnsignedIntegerType())
161+
return;
162+
163+
// Get a printable type string
164+
std::string TypeStr = QT.getUnqualifiedType().getAsString();
165+
if (const auto *Typedef = QT->getAs<TypedefType>()) {
166+
TypeStr = Typedef->getDecl()->getName().str();
167+
}
168+
169+
// Get original source text for diagnostic message
170+
StringRef OriginalText =
171+
Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
172+
SM, getLangOpts());
173+
174+
// Build replacement text
175+
std::string Replacement = "std::numeric_limits<" + TypeStr + ">::max()";
176+
177+
// Create diagnostic
178+
auto Diag = diag(E->getBeginLoc(),
179+
"use 'std::numeric_limits<%0>::max()' instead of '%1'")
180+
<< TypeStr << OriginalText;
181+
182+
// Add fix-it hints
183+
Diag << FixItHint::CreateReplacement(E->getSourceRange(), Replacement);
184+
185+
// Add include for <limits>
186+
FileID FID = SM.getFileID(E->getBeginLoc());
187+
if (auto IncludeHint = Inserter.createIncludeInsertion(FID, "<limits>")) {
188+
Diag << *IncludeHint;
189+
}
190+
}
191+
192+
} // namespace clang::tidy::readability

‎clang-tools-extra/clang-tidy/readability/NumericlimitmaxcheckCheck.h‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NUMERICLIMITMAXCHECKCHECK_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NUMERICLIMITMAXCHECKCHECK_H
11+
12+
#include "../ClangTidyCheck.h"
13+
#include "../utils/IncludeInserter.h"
14+
15+
namespace clang::tidy::readability {
16+
17+
/// Suggest replacement of values like -1 / `~0` (when used as unsigned)
18+
/// with std::numeric_limits<T>::max() for unsigned integer types.
19+
class NumericlimitmaxcheckCheck : public ClangTidyCheck {
20+
public:
21+
NumericlimitmaxcheckCheck(StringRef Name, ClangTidyContext *Context);
22+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
23+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
24+
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
25+
26+
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpander) override;
27+
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
28+
29+
private:
30+
utils::IncludeInserter Inserter;
31+
};
32+
33+
} // namespace clang::tidy::readability
34+
35+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NUMERICLIMITMAXCHECKCHECK_H

‎clang-tools-extra/test/clang-tidy/checkers/readability/NumericLimitMaxCheck.cpp‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// RUN: %check_clang_tidy %s readability-NumericLimitMaxCheck %t
2+
3+
// Defines a typedef, which the check should respect and use in its fix.
4+
typedef unsigned long long my_uint_t;
5+
6+
void test_arg(unsigned int);
7+
8+
void test_unsigned_literals() {
9+
10+
unsigned int a = -1;
11+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
12+
// CHECK-FIXES: unsigned int a = std::numeric_limits<unsigned int>::max();
13+
14+
unsigned int b = ~0;
15+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '~0' [readability-NumericLimitMaxCheck]
16+
// CHECK-FIXES: unsigned int b = std::numeric_limits<unsigned int>::max();
17+
18+
unsigned long c = (unsigned long)(-1);
19+
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'std::numeric_limits<unsigned long>::max()' instead of '(unsigned long)(-1)' [readability-NumericLimitMaxCheck]
20+
// CHECK-FIXES: unsigned long c = std::numeric_limits<unsigned long>::max();
21+
22+
unsigned long d = (unsigned long)(~0);
23+
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'std::numeric_limits<unsigned long>::max()' instead of '(unsigned long)(~0)' [readability-NumericLimitMaxCheck]
24+
// CHECK-FIXES: unsigned long d = std::numeric_limits<unsigned long>::max();
25+
26+
}
27+
28+
void test_comparisons(unsigned x) {
29+
if (x == -1) {
30+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
31+
// CHECK-FIXES: if (x == std::numeric_limits<unsigned int>::max()) {
32+
;
33+
}
34+
35+
if (x == (unsigned)(~0));
36+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '(unsigned)(~0)' [readability-NumericLimitMaxCheck]
37+
// CHECK-FIXES: if (x == std::numeric_limits<unsigned int>::max());
38+
39+
}
40+
41+
void test_cpp_casts_and_other_types() {
42+
unsigned int e = static_cast<unsigned int>(-1);
43+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of 'static_cast<unsigned int>(-1)' [readability-NumericLimitMaxCheck]
44+
// CHECK-FIXES: unsigned int e = std::numeric_limits<unsigned int>::max();
45+
46+
unsigned int f = unsigned(-1);
47+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of 'unsigned(-1)' [readability-NumericLimitMaxCheck]
48+
// CHECK-FIXES: unsigned int f = std::numeric_limits<unsigned int>::max();
49+
50+
unsigned short s = -1;
51+
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: use 'std::numeric_limits<unsigned short>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
52+
// CHECK-FIXES: unsigned short s = std::numeric_limits<unsigned short>::max();
53+
54+
unsigned char c = ~0;
55+
// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'std::numeric_limits<unsigned char>::max()' instead of '~0' [readability-NumericLimitMaxCheck]
56+
// CHECK-FIXES: unsigned char c = std::numeric_limits<unsigned char>::max();
57+
58+
// Tests that the check correctly uses the typedef'd name "my_uint_t"
59+
my_uint_t u = -1;
60+
// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use 'std::numeric_limits<my_uint_t>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
61+
// CHECK-FIXES: my_uint_t u = std::numeric_limits<my_uint_t>::max();
62+
}
63+
64+
unsigned test_other_contexts(bool x) {
65+
// Test function arguments
66+
test_arg(-1);
67+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
68+
// CHECK-FIXES: test_arg(std::numeric_limits<unsigned int>::max());
69+
70+
// Test ternary operator
71+
unsigned k = x ? -1 : 0;
72+
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
73+
// CHECK-FIXES: unsigned k = x ? std::numeric_limits<unsigned int>::max() : 0;
74+
75+
unsigned k2 = x ? 0 : ~0;
76+
// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '~0' [readability-NumericLimitMaxCheck]
77+
// CHECK-FIXES: unsigned k2 = x ? 0 : std::numeric_limits<unsigned int>::max();
78+
79+
// Test return values
80+
return -1;
81+
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'std::numeric_limits<unsigned int>::max()' instead of '-1' [readability-NumericLimitMaxCheck]
82+
// CHECK-FIXES: return std::numeric_limits<unsigned int>::max();
83+
}
84+
85+
86+
#define MY_MAX_MACRO -1
87+
#define MY_MAX_MACRO_TILDE ~0
88+
89+
void test_no_warning() {
90+
int a = -1;
91+
unsigned u = 0;
92+
int b = ~0;
93+
unsigned c = 42;
94+
95+
// This is (max - 1), not max
96+
unsigned d = (unsigned)-2;
97+
98+
// The check should ignore code from macros
99+
unsigned m = MY_MAX_MACRO;
100+
unsigned m2 = MY_MAX_MACRO_TILDE;
101+
}

0 commit comments

Comments
 (0)