llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Nicolas van Kempen (nicovank)
<details>
<summary>Changes</summary>
Using `compare` is the next most common roundabout way to express `starts_with`
before it was added to the standard. In this case, using `starts_with` is a
readability improvement. Extend existing `modernize-use-starts-ends-with` to
cover this case.
```
// The following will now be replaced by starts_with().
string.compare(0, strlen("prefix"), "prefix") == 0;
string.compare(0, 6, "prefix") == 0;
string.compare(0, prefix.length(), prefix) == 0;
string.compare(0, prefix.size(), prefix) == 0;
```
There are no such instances in llvm-project, maybe more will surface when the
C++ default is changed to 20 for `std::string`. Other build issues come up when
trying to override it.
Running this on llvm-project and dolphin:
- https://github.com/llvm/llvm-project/pull/89140 (no additional instances)
- https://github.com/dolphin-emu/dolphin/pull/12718
---
Full diff: https://github.com/llvm/llvm-project/pull/89530.diff
7 Files Affected:
- (modified) clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
(+77-15)
- (modified) clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
(+2-2)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
- (modified)
clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
(+4-2)
- (modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
(+4)
- (modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h
(+1)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
(+45)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 062f6e9911dbed..38fe1984ac494e 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -16,6 +16,49 @@
using namespace clang::ast_matchers;
namespace clang::tidy::modernize {
+namespace {
+// Given two argument indices X and Y, matches when a call expression has a
+// string at index X with an expression representing that string's length at
+// index Y. The string can be a string literal or a variable. The length can be
+// matched via an integer literal or a call to strlen() in the case of a string
+// literal, and by a call to size() or length() in the string variable case.
+AST_POLYMORPHIC_MATCHER_P2(HasStringAndLengthArguments,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(
+ CallExpr, CXXConstructExpr,
+ CXXUnresolvedConstructExpr, ObjCMessageExpr),
+ unsigned, StringArgIndex, unsigned, LengthArgIndex)
{
+ if (StringArgIndex >= Node.getNumArgs() ||
+ LengthArgIndex >= Node.getNumArgs()) {
+ return false;
+ }
+
+ const Expr *StringArgExpr =
+ Node.getArg(StringArgIndex)->IgnoreParenImpCasts();
+ const Expr *LengthArgExpr =
+ Node.getArg(LengthArgIndex)->IgnoreParenImpCasts();
+
+ if (const auto *StringArg = dyn_cast<StringLiteral>(StringArgExpr)) {
+ // Match an integer literal equal to the string length or a call to strlen.
+ const auto Matcher = expr(anyOf(
+ integerLiteral(equals(StringArg->getLength())),
+ callExpr(
+ callee(functionDecl(hasName("strlen"))), argumentCountIs(1),
+ hasArgument(0, stringLiteral(hasSize(StringArg->getLength()))))));
+ return Matcher.matches(*LengthArgExpr, Finder, Builder);
+ }
+
+ if (const auto *StringArg = dyn_cast<DeclRefExpr>(StringArgExpr)) {
+ // Match a call to size() or length() on the same variable.
+ const auto Matcher = cxxMemberCallExpr(
+ on(declRefExpr(to(varDecl(equalsNode(StringArg->getDecl()))))),
+ callee(cxxMethodDecl(hasAnyName("size", "length"), isConst(),
+ parameterCountIs(0))));
+ return Matcher.matches(*LengthArgExpr, Finder, Builder);
+ }
+
+ return false;
+}
+} // namespace
UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
ClangTidyContext *Context)
@@ -43,7 +86,9 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder
*Finder) {
callee(cxxMethodDecl(hasName("find")).bind("find_fun")),
// ... on a class with a starts_with function.
on(hasType(
- hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))));
+ hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
+ // Bind search expression.
+ hasArgument(0, expr().bind("search_expr")));
const auto RFindExpr = cxxMemberCallExpr(
// A method call with a second argument of zero...
@@ -52,15 +97,30 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder
*Finder) {
callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
// ... on a class with a starts_with function.
on(hasType(
- hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))));
-
- const auto FindOrRFindExpr =
- cxxMemberCallExpr(anyOf(FindExpr, RFindExpr)).bind("find_expr");
+ hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
+ // Bind search expression.
+ hasArgument(0, expr().bind("search_expr")));
+
+ const auto CompareExpr = cxxMemberCallExpr(
+ // A method call with a first argument of zero...
+ hasArgument(0, ZeroLiteral),
+ // ... named compare...
+ callee(cxxMethodDecl(hasName("compare")).bind("find_fun")),
+ // ... on a class with a starts_with function...
+ on(hasType(
+ hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
+ // ... where the third argument is some string and the second its length.
+ HasStringAndLengthArguments(2, 1),
+ // Bind search expression.
+ hasArgument(2, expr().bind("search_expr")));
Finder->addMatcher(
- // Match [=!]= with a zero on one side and a string.(r?)find on the
other.
- binaryOperator(hasAnyOperatorName("==", "!="),
- hasOperands(FindOrRFindExpr, ZeroLiteral))
+ // Match [=!]= with a zero on one side and (r?)find|compare on the other.
+ binaryOperator(
+ hasAnyOperatorName("==", "!="),
+ hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr,
CompareExpr))
+ .bind("find_expr"),
+ ZeroLiteral))
.bind("expr"),
this);
}
@@ -69,6 +129,7 @@ void UseStartsEndsWithCheck::check(const
MatchFinder::MatchResult &Result) {
const auto *ComparisonExpr = Result.Nodes.getNodeAs<BinaryOperator>("expr");
const auto *FindExpr =
Result.Nodes.getNodeAs<CXXMemberCallExpr>("find_expr");
const auto *FindFun = Result.Nodes.getNodeAs<CXXMethodDecl>("find_fun");
+ const auto *SearchExpr = Result.Nodes.getNodeAs<Expr>("search_expr");
const auto *StartsWithFunction =
Result.Nodes.getNodeAs<CXXMethodDecl>("starts_with_fun");
@@ -79,13 +140,13 @@ void UseStartsEndsWithCheck::check(const
MatchFinder::MatchResult &Result) {
const bool Neg = ComparisonExpr->getOpcode() == BO_NE;
auto Diagnostic =
- diag(FindExpr->getBeginLoc(), "use %0 instead of %1() %select{==|!=}2 0")
+ diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0")
<< StartsWithFunction->getName() << FindFun->getName() << Neg;
- // Remove possible zero second argument and ' [!=]= 0' suffix.
+ // Remove possible arguments after search expression and ' [!=]= 0' suffix.
Diagnostic << FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(
- Lexer::getLocForEndOfToken(FindExpr->getArg(0)->getEndLoc(), 0,
+ Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
*Result.SourceManager, getLangOpts()),
ComparisonExpr->getEndLoc()),
")");
@@ -94,11 +155,12 @@ void UseStartsEndsWithCheck::check(const
MatchFinder::MatchResult &Result) {
Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc()));
- // Replace '(r?)find' with 'starts_with'.
+ // Replace method name by 'starts_with'.
+ // Remove possible arguments before search expression.
Diagnostic << FixItHint::CreateReplacement(
- CharSourceRange::getTokenRange(FindExpr->getExprLoc(),
- FindExpr->getExprLoc()),
- StartsWithFunction->getName());
+ CharSourceRange::getCharRange(FindExpr->getExprLoc(),
+ SearchExpr->getBeginLoc()),
+ StartsWithFunction->getNameAsString() + "(");
// Add possible negation '!'.
if (Neg) {
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
index 34e97177682575..0186d2330e6d6f 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
@@ -13,8 +13,8 @@
namespace clang::tidy::modernize {
-/// Checks whether a ``find`` or ``rfind`` result is compared with 0 and
-/// suggests replacing with ``starts_with`` when the method exists in the
class.
+/// Checks for common roundabout ways to express `starts_with` and `ends_with`
+/// and suggests replacing with ``starts_with`` when the method is available.
/// Notably, this will work with ``std::string`` and ``std::string_view``.
///
/// For the user-facing documentation see:
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst
b/clang-tools-extra/docs/ReleaseNotes.rst
index 9ef1d38d3c4560..a33800c4fbb31c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -298,6 +298,10 @@ Changes in existing checks
check by resolving fix-it overlaps in template code by disregarding implicit
instances.
+- Improved :doc:`modernize-use-starts-ends-with
+ <clang-tidy/checks/modernize/use-starts-ends-with>` check to also handle
+ cases using `compare()`.
+
Removed checks
^^^^^^^^^^^^^^
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
index 7f8a262d2ab3aa..7167937427bd09 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
@@ -3,8 +3,8 @@
modernize-use-starts-ends-with
==============================
-Checks whether a ``find`` or ``rfind`` result is compared with 0 and suggests
-replacing with ``starts_with`` when the method exists in the class. Notably,
+Checks for common roundabout ways to express `starts_with` and `ends_with` and
+suggests replacing with ``starts_with`` when the method is available. Notably,
this will work with ``std::string`` and ``std::string_view``.
.. code-block:: c++
@@ -12,6 +12,7 @@ this will work with ``std::string`` and ``std::string_view``.
std::string s = "...";
if (s.find("prefix") == 0) { /* do something */ }
if (s.rfind("prefix", 0) == 0) { /* do something */ }
+ if (s.compare(0, strlen("prefix"), "prefix") == 0) { /* do something */ }
becomes
@@ -20,3 +21,4 @@ becomes
std::string s = "...";
if (s.starts_with("prefix")) { /* do something */ }
if (s.starts_with("prefix")) { /* do something */ }
+ if (s.starts_with("prefix")) { /* do something */ }
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
index 28e2b4a231e52e..d031f27beb9dfe 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -44,6 +44,8 @@ struct basic_string {
int compare(const C* s) const;
int compare(size_type pos, size_type len, const _Type&) const;
int compare(size_type pos, size_type len, const C* s) const;
+ template<class StringViewLike>
+ int compare(size_type pos1, size_type count1, const StringViewLike& t) const;
size_type find(const _Type& str, size_type pos = 0) const;
size_type find(const C* s, size_type pos = 0) const;
@@ -129,6 +131,8 @@ bool operator!=(const char*, const std::string&);
bool operator==(const std::wstring&, const std::wstring&);
bool operator==(const std::wstring&, const wchar_t*);
bool operator==(const wchar_t*, const std::wstring&);
+
+size_t strlen(const char* str);
}
#endif // _STRING_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h
b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h
index 4ab7e930e4b506..af205868059a82 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h
@@ -12,5 +12,6 @@
#include "stddef.h"
void *memcpy(void *dest, const void *src, size_t n);
+size_t strlen(const char* str);
#endif // _STRING_H_
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
index 65ed9ed895bc47..4cfeed31445c2d 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp
@@ -1,6 +1,7 @@
// RUN: %check_clang_tidy -std=c++20 %s modernize-use-starts-ends-with %t -- \
// RUN: -- -isystem %clang_tidy_headers
+#include <string.h>
#include <string>
std::string foo(std::string);
@@ -158,10 +159,54 @@ void test(std::string s, std::string_view sv, sub_string
ss, sub_sub_string sss,
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use startsWith
// CHECK-FIXES: puvi.startsWith("a");
+ s.compare(0, 1, "a") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead
of compare() == 0
+ // CHECK-FIXES: s.starts_with("a");
+
+ s.compare(0, 1, "a") != 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead
of compare() != 0
+ // CHECK-FIXES: !s.starts_with("a");
+
+ s.compare(0, strlen("a"), "a") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with("a");
+
+ s.compare(0, std::strlen("a"), "a") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with("a");
+
+ s.compare(0, s.size(), s) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
+ s.compare(0, s.length(), s) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
+ 0 != s.compare(0, sv.length(), sv);
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(sv);
+
+ #define LENGTH(x) (x).length()
+ s.compare(0, LENGTH(s), s) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
+ s.compare(ZERO, LENGTH(s), s) == ZERO;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: s.starts_with(s);
+
+ s.compare(ZERO, LENGTH(sv), sv) != 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
+ // CHECK-FIXES: !s.starts_with(sv);
+
// Expressions that don't trigger the check are here.
#define EQ(x, y) ((x) == (y))
EQ(s.find("a"), 0);
#define DOTFIND(x, y) (x).find(y)
DOTFIND(s, "a") == 0;
+
+ #define STARTS_WITH_COMPARE(x, y) (x).compare(0, (x).size(), (y))
+ STARTS_WITH_COMPARE(s, s) == 0;
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/89530
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
