Skip to content

Commit 5084482

Browse files
hjanuschkahjanuschka
authored
[clang-tidy] Enhance modernize-use-starts-ends-with to handle substr patterns (llvm#116033)
Enhances the modernize-use-starts-ends-with check to detect additional patterns using substr that can be replaced with starts_with() (C++20). This enhancement improves code readability and can be more efficient by avoiding temporary string creation.
1 parent cc11310 commit 5084482

File tree

5 files changed

+136
-59
lines changed

5 files changed

+136
-59
lines changed

‎clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp‎

Lines changed: 51 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,17 @@
1818
using namespace clang::ast_matchers;
1919

2020
namespace clang::tidy::modernize {
21+
22+
static bool isNegativeComparison(const Expr *ComparisonExpr) {
23+
if (const auto *BO = llvm::dyn_cast<BinaryOperator>(ComparisonExpr))
24+
return BO->getOpcode() == BO_NE;
25+
26+
if (const auto *Op = llvm::dyn_cast<CXXOperatorCallExpr>(ComparisonExpr))
27+
return Op->getOperator() == OO_ExclaimEqual;
28+
29+
return false;
30+
}
31+
2132
struct NotLengthExprForStringNode {
2233
NotLengthExprForStringNode(std::string ID, DynTypedNode Node,
2334
ASTContext *Context)
@@ -171,10 +182,26 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
171182
hasRHS(lengthExprForStringNode("needle")))))
172183
.bind("expr"),
173184
this);
185+
186+
// Case 6: X.substr(0, LEN(Y)) [!=]= Y -> starts_with.
187+
Finder->addMatcher(
188+
cxxOperatorCallExpr(
189+
hasAnyOperatorName("==", "!="),
190+
hasOperands(
191+
expr().bind("needle"),
192+
cxxMemberCallExpr(
193+
argumentCountIs(2), hasArgument(0, ZeroLiteral),
194+
hasArgument(1, lengthExprForStringNode("needle")),
195+
callee(cxxMethodDecl(hasName("substr"),
196+
ofClass(OnClassWithStartsWithFunction))
197+
.bind("find_fun")))
198+
.bind("find_expr")))
199+
.bind("expr"),
200+
this);
174201
}
175202

176203
void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
177-
const auto *ComparisonExpr = Result.Nodes.getNodeAs<BinaryOperator>("expr");
204+
const auto *ComparisonExpr = Result.Nodes.getNodeAs<Expr>("expr");
178205
const auto *FindExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("find_expr");
179206
const auto *FindFun = Result.Nodes.getNodeAs<CXXMethodDecl>("find_fun");
180207
const auto *SearchExpr = Result.Nodes.getNodeAs<Expr>("needle");
@@ -183,39 +210,42 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
183210
const auto *EndsWithFunction =
184211
Result.Nodes.getNodeAs<CXXMethodDecl>("ends_with_fun");
185212
assert(bool(StartsWithFunction) != bool(EndsWithFunction));
213+
186214
const CXXMethodDecl *ReplacementFunction =
187215
StartsWithFunction ? StartsWithFunction : EndsWithFunction;
188216

189-
if (ComparisonExpr->getBeginLoc().isMacroID())
217+
if (ComparisonExpr->getBeginLoc().isMacroID() ||
218+
FindExpr->getBeginLoc().isMacroID())
190219
return;
191220

192-
const bool Neg = ComparisonExpr->getOpcode() == BO_NE;
221+
// Make sure FindExpr->getArg(0) can be used to make a range in the FitItHint.
222+
if (FindExpr->getNumArgs() == 0)
223+
return;
193224

194-
auto Diagnostic =
195-
diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0")
196-
<< ReplacementFunction->getName() << FindFun->getName() << Neg;
225+
// Retrieve the source text of the search expression.
226+
const auto SearchExprText = Lexer::getSourceText(
227+
CharSourceRange::getTokenRange(SearchExpr->getSourceRange()),
228+
*Result.SourceManager, Result.Context->getLangOpts());
197229

198-
// Remove possible arguments after search expression and ' [!=]= .+' suffix.
199-
Diagnostic << FixItHint::CreateReplacement(
200-
CharSourceRange::getTokenRange(
201-
Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
202-
*Result.SourceManager, getLangOpts()),
203-
ComparisonExpr->getEndLoc()),
204-
")");
230+
auto Diagnostic = diag(FindExpr->getExprLoc(), "use %0 instead of %1")
231+
<< ReplacementFunction->getName() << FindFun->getName();
205232

206-
// Remove possible '.+ [!=]= ' prefix.
233+
// Remove everything before the function call.
207234
Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
208235
ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc()));
209236

210-
// Replace method name by '(starts|ends)_with'.
211-
// Remove possible arguments before search expression.
237+
// Rename the function to `starts_with` or `ends_with`.
238+
Diagnostic << FixItHint::CreateReplacement(FindExpr->getExprLoc(),
239+
ReplacementFunction->getName());
240+
241+
// Replace arguments and everything after the function call.
212242
Diagnostic << FixItHint::CreateReplacement(
213-
CharSourceRange::getCharRange(FindExpr->getExprLoc(),
214-
SearchExpr->getBeginLoc()),
215-
(ReplacementFunction->getName() + "(").str());
243+
CharSourceRange::getTokenRange(FindExpr->getArg(0)->getBeginLoc(),
244+
ComparisonExpr->getEndLoc()),
245+
(SearchExprText + ")").str());
216246

217-
// Add possible negation '!'.
218-
if (Neg)
247+
// Add negation if necessary.
248+
if (isNegativeComparison(ComparisonExpr))
219249
Diagnostic << FixItHint::CreateInsertion(FindExpr->getBeginLoc(), "!");
220250
}
221251

‎clang-tools-extra/docs/ReleaseNotes.rst‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,8 +262,9 @@ Changes in existing checks
262262
``NULL``/``__null`` (but not ``0``) when used with a templated type.
263263

264264
- Improved :doc:`modernize-use-starts-ends-with
265-
<clang-tidy/checks/modernize/use-starts-ends-with>` check to handle two cases
266-
that can be replaced with ``ends_with``
265+
<clang-tidy/checks/modernize/use-starts-ends-with>` check to handle two new
266+
cases from ``rfind`` and ``compare`` to ``ends_with``, and one new case from
267+
``substr`` to ``starts_with``, and a small adjustment to the diagnostic message.
267268

268269
- Improved :doc:`modernize-use-std-format
269270
<clang-tidy/checks/modernize/use-std-format>` check to support replacing

‎clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,26 +7,16 @@ Checks for common roundabout ways to express ``starts_with`` and ``ends_with``
77
and suggests replacing with the simpler method when it is available. Notably,
88
this will work with ``std::string`` and ``std::string_view``.
99

10-
.. code-block:: c++
10+
Covered scenarios:
1111

12-
std::string s = "...";
13-
if (s.find("prefix") == 0) { /* do something */ }
14-
if (s.rfind("prefix", 0) == 0) { /* do something */ }
15-
if (s.compare(0, strlen("prefix"), "prefix") == 0) { /* do something */ }
16-
if (s.compare(s.size() - strlen("suffix"), strlen("suffix"), "suffix") == 0) {
17-
/* do something */
18-
}
19-
if (s.rfind("suffix") == (s.length() - 6)) {
20-
/* do something */
21-
}
22-
23-
becomes
24-
25-
.. code-block:: c++
26-
27-
std::string s = "...";
28-
if (s.starts_with("prefix")) { /* do something */ }
29-
if (s.starts_with("prefix")) { /* do something */ }
30-
if (s.starts_with("prefix")) { /* do something */ }
31-
if (s.ends_with("suffix")) { /* do something */ }
32-
if (s.ends_with("suffix")) { /* do something */ }
12+
==================================================== =====================
13+
Expression Replacement
14+
---------------------------------------------------- ---------------------
15+
``u.find(v) == 0`` ``u.starts_with(v)``
16+
``u.rfind(v, 0) != 0`` ``!u.starts_with(v)``
17+
``u.compare(0, v.size(), v) == 0`` ``u.starts_with(v)``
18+
``u.substr(0, v.size()) == v`` ``u.starts_with(v)``
19+
``v != u.substr(0, v.size())`` ``!u.starts_with(v)``
20+
``u.compare(u.size() - v.size(), v.size(), v) == 0`` ``u.ends_with(v)``
21+
``u.rfind(v) == u.size() - v.size()`` ``u.ends_with(v)``
22+
==================================================== =====================

‎clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ struct basic_string {
6060
_Type& insert(size_type pos, const C* s);
6161
_Type& insert(size_type pos, const C* s, size_type n);
6262

63+
_Type substr(size_type pos = 0, size_type count = npos) const;
64+
6365
constexpr bool starts_with(std::basic_string_view<C, T> sv) const noexcept;
6466
constexpr bool starts_with(C ch) const noexcept;
6567
constexpr bool starts_with(const C* s) const;

‎clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp‎

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
3636
string_like sl, string_like_camel slc, prefer_underscore_version puv,
3737
prefer_underscore_version_flip puvf) {
3838
s.find("a") == 0;
39-
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of find() == 0
39+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of find [modernize-use-starts-ends-with]
4040
// CHECK-FIXES: s.starts_with("a");
4141

4242
(((((s)).find("a")))) == ((0));
@@ -68,7 +68,7 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
6868
// CHECK-FIXES: !s.starts_with("a");
6969

7070
s.rfind("a", 0) == 0;
71-
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of rfind() == 0
71+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of rfind [modernize-use-starts-ends-with]
7272
// CHECK-FIXES: s.starts_with("a");
7373

7474
s.rfind(s, 0) == 0;
@@ -91,16 +91,6 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
9191
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
9292
// CHECK-FIXES: !s.starts_with("a");
9393

94-
#define STR(x) std::string(x)
95-
0 == STR(s).find("a");
96-
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
97-
// CHECK-FIXES: STR(s).starts_with("a");
98-
99-
#define STRING s
100-
if (0 == STRING.find("ala")) { /* do something */}
101-
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
102-
// CHECK-FIXES: if (STRING.starts_with("ala"))
103-
10494
#define FIND find
10595
s.FIND("a") == 0;
10696
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
@@ -149,11 +139,11 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
149139
// CHECK-FIXES: puvf.starts_with("a");
150140

151141
s.compare(0, 1, "a") == 0;
152-
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare() == 0
142+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare [modernize-use-starts-ends-with]
153143
// CHECK-FIXES: s.starts_with("a");
154144

155145
s.compare(0, 1, "a") != 0;
156-
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare() != 0
146+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare [modernize-use-starts-ends-with]
157147
// CHECK-FIXES: !s.starts_with("a");
158148

159149
s.compare(0, strlen("a"), "a") == 0;
@@ -265,4 +255,68 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
265255

266256
s.compare(0, 1, "ab") == 0;
267257
s.rfind(suffix, 1) == s.size() - suffix.size();
258+
259+
#define STR(x) std::string(x)
260+
0 == STR(s).find("a");
261+
262+
#define STRING s
263+
if (0 == STRING.find("ala")) { /* do something */}
264+
}
265+
266+
void test_substr() {
267+
std::string str("hello world");
268+
std::string prefix = "hello";
269+
270+
// Basic pattern
271+
str.substr(0, 5) == "hello";
272+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
273+
// CHECK-FIXES: str.starts_with("hello");
274+
275+
// With string literal on left side
276+
"hello" == str.substr(0, 5);
277+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
278+
// CHECK-FIXES: str.starts_with("hello");
279+
280+
// Inequality comparison
281+
str.substr(0, 5) != "world";
282+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
283+
// CHECK-FIXES: !str.starts_with("world");
284+
285+
// Ensure non-zero start position is not transformed
286+
str.substr(1, 5) == "hello";
287+
str.substr(0, 4) == "hello"; // Length mismatch
288+
289+
size_t len = 5;
290+
str.substr(0, len) == "hello"; // Non-constant length
291+
292+
// String literal with size calculation
293+
str.substr(0, strlen("hello")) == "hello";
294+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
295+
// CHECK-FIXES: str.starts_with("hello");
296+
297+
str.substr(0, prefix.size()) == prefix;
298+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
299+
// CHECK-FIXES: str.starts_with(prefix);
300+
301+
str.substr(0, prefix.length()) == prefix;
302+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
303+
// CHECK-FIXES: str.starts_with(prefix);
304+
305+
// Tests to verify macro behavior
306+
#define MSG "hello"
307+
str.substr(0, strlen(MSG)) == MSG;
308+
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with]
309+
// CHECK-FIXES: str.starts_with(MSG);
310+
311+
#define STARTS_WITH(X, Y) (X).substr(0, (Y).size()) == (Y)
312+
STARTS_WITH(str, prefix);
313+
314+
#define SUBSTR(X, A, B) (X).substr((A), (B))
315+
SUBSTR(str, 0, 6) == "prefix";
316+
317+
#define STR() str
318+
SUBSTR(STR(), 0, 6) == "prefix";
319+
"prefix" == SUBSTR(STR(), 0, 6);
320+
321+
str.substr(0, strlen("hello123")) == "hello";
268322
}

0 commit comments

Comments
 (0)