[clang-tidy][modernize-use-starts-ends-with] Add support for compare() #89530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra Author: Nicolas van Kempen (nicovank) ChangesUsing There are no such instances in llvm-project, maybe more will surface when the C++ default is changed to 20 for Running this on llvm-project and dolphin:
Full diff: https://github.com/llvm/llvm-project/pull/89530.diff 7 Files Affected:
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;
}
|
|
@llvm/pr-subscribers-clang-tidy Author: Nicolas van Kempen (nicovank) ChangesUsing There are no such instances in llvm-project, maybe more will surface when the C++ default is changed to 20 for Running this on llvm-project and dolphin:
Full diff: https://github.com/llvm/llvm-project/pull/89530.diff 7 Files Affected:
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;
}
|
nicovank
force-pushed
the
starts-with-compare
branch
2 times, most recently
from
5f20627
to
7a2ff84
Compare
PiotrZSL
approved these changes
Apr 21, 2024
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
nicovank
force-pushed
the
starts-with-compare
branch
from
7a2ff84
to
ff7ebb3
Compare
|
Update following feedback. I rewrote |
PiotrZSL
reviewed
Apr 21, 2024
clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
Outdated
Show resolved
Hide resolved
nicovank
force-pushed
the
starts-with-compare
branch
from
ff7ebb3
to
98af89a
Compare
|
Got it, thanks! I'm somewhat familiar using InnerMatchers but didn't think that solution fit quite right here. This new version I think is much better. Kept matcher construction in |
clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst
Outdated
Show resolved
Hide resolved
nicovank
force-pushed
the
starts-with-compare
branch
from
98af89a
to
c670692
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using
compareis the next most common roundabout way to expressstarts_withbefore it was added to the standard. In this case, usingstarts_withis a readability improvement. Extend existingmodernize-use-starts-ends-withto cover this case.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: