[clang-tidy][modernize-use-starts-ends-with] Add support for two ends_with patterns #105366
Conversation
This file contains hidden or 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) ChangesAdd support for the following two patterns which are found occasionally, that can be replaced with haystack.compare(haystack.length() - needle.length(), needle.length(), needle) == 0;
haystack.rfind(needle) == (haystack.size() - needle.size());A big change is adding a Patch is 21.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105366.diff 6 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 89ee45faecd7f3..5eb3267adb0799 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -8,6 +8,7 @@
#include "UseStartsEndsWithCheck.h"
+#include "../utils/ASTUtils.h"
#include "../utils/OptionsUtils.h"
#include "clang/Lex/Lexer.h"
@@ -16,6 +17,63 @@
using namespace clang::ast_matchers;
namespace clang::tidy::modernize {
+struct NotLengthExprForStringNode {
+ NotLengthExprForStringNode(std::string ID, DynTypedNode Node,
+ ASTContext *Context)
+ : ID(std::move(ID)), Node(std::move(Node)), Context(Context) {}
+ bool operator()(const internal::BoundNodesMap &Nodes) const {
+ // Match a string literal and an integer size or strlen() call.
+ if (const auto *StringLiteralNode = Nodes.getNodeAs<StringLiteral>(ID)) {
+ if (const auto *IntegerLiteralSizeNode = Node.get<IntegerLiteral>()) {
+ return StringLiteralNode->getLength() !=
+ IntegerLiteralSizeNode->getValue().getZExtValue();
+ }
+
+ if (const auto *StrlenNode = Node.get<CallExpr>()) {
+ if (StrlenNode->getDirectCallee()->getName() != "strlen" ||
+ StrlenNode->getNumArgs() != 1) {
+ return true;
+ }
+
+ if (const auto *StrlenArgNode = dyn_cast<StringLiteral>(
+ StrlenNode->getArg(0)->IgnoreParenImpCasts())) {
+ return StrlenArgNode->getLength() != StringLiteralNode->getLength();
+ }
+ }
+ }
+
+ // Match a string variable and a call to length() or size().
+ if (const auto *ExprNode = Nodes.getNodeAs<Expr>(ID)) {
+ if (const auto *MemberCallNode = Node.get<CXXMemberCallExpr>()) {
+ const CXXMethodDecl *MethodDeclNode = MemberCallNode->getMethodDecl();
+ const StringRef Name = MethodDeclNode->getName();
+ if (!MethodDeclNode->isConst() || MethodDeclNode->getNumParams() != 0 ||
+ (Name != "size" && Name != "length")) {
+ return true;
+ }
+
+ if (const auto *OnNode =
+ dyn_cast<Expr>(MemberCallNode->getImplicitObjectArgument())) {
+ return !utils::areStatementsIdentical(OnNode->IgnoreParenImpCasts(),
+ ExprNode->IgnoreParenImpCasts(),
+ *Context);
+ }
+ }
+ }
+
+ return true;
+ }
+
+private:
+ std::string ID;
+ DynTypedNode Node;
+ ASTContext *Context;
+};
+
+AST_MATCHER_P(Expr, lengthExprForStringNode, std::string, ID) {
+ return Builder->removeBindings(NotLengthExprForStringNode(
+ ID, DynTypedNode::create(Node), &(Finder->getASTContext())));
+}
UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
ClangTidyContext *Context)
@@ -23,6 +81,7 @@ UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
const auto ZeroLiteral = integerLiteral(equals(0));
+
const auto HasStartsWithMethodWithName = [](const std::string &Name) {
return hasMethod(
cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
@@ -32,119 +91,104 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
anyOf(HasStartsWithMethodWithName("starts_with"),
HasStartsWithMethodWithName("startsWith"),
HasStartsWithMethodWithName("startswith"));
- const auto ClassWithStartsWithFunction = cxxRecordDecl(anyOf(
- HasStartsWithMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
- cxxRecordDecl(HasStartsWithMethod)))))));
+ const auto OnClassWithStartsWithFunction =
+ on(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+ anyOf(HasStartsWithMethod,
+ hasAnyBase(hasType(hasCanonicalType(
+ hasDeclaration(cxxRecordDecl(HasStartsWithMethod)))))))))));
+ const auto HasEndsWithMethodWithName = [](const std::string &Name) {
+ return hasMethod(
+ cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
+ .bind("ends_with_fun"));
+ };
+ const auto HasEndsWithMethod = anyOf(HasEndsWithMethodWithName("ends_with"),
+ HasEndsWithMethodWithName("endsWith"),
+ HasEndsWithMethodWithName("endswith"));
+ const auto OnClassWithEndsWithFunction =
+ on(expr(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+ anyOf(HasEndsWithMethod,
+ hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
+ cxxRecordDecl(HasEndsWithMethod)))))))))))
+ .bind("haystack"));
+
+ // Case 1: X.find(Y) [!=]= 0 -> starts_with.
const auto FindExpr = cxxMemberCallExpr(
- // A method call with no second argument or the second argument is zero...
anyOf(argumentCountIs(1), hasArgument(1, ZeroLiteral)),
- // ... named find...
callee(cxxMethodDecl(hasName("find")).bind("find_fun")),
- // ... on a class with a starts_with function.
- on(hasType(
- hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
- // Bind search expression.
- hasArgument(0, expr().bind("search_expr")));
+ OnClassWithStartsWithFunction, hasArgument(0, expr().bind("needle")));
+ // Case 2: X.rfind(Y, 0) [!=]= 0 -> starts_with.
const auto RFindExpr = cxxMemberCallExpr(
- // A method call with a second argument of zero...
hasArgument(1, ZeroLiteral),
- // ... named rfind...
callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
- // ... on a class with a starts_with function.
- on(hasType(
- hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
- // Bind search expression.
- hasArgument(0, expr().bind("search_expr")));
-
- // Match a string literal and an integer or strlen() call matching the length.
- const auto HasStringLiteralAndLengthArgs = [](const auto StringArgIndex,
- const auto LengthArgIndex) {
- return allOf(
- hasArgument(StringArgIndex, stringLiteral().bind("string_literal_arg")),
- hasArgument(LengthArgIndex,
- anyOf(integerLiteral().bind("integer_literal_size_arg"),
- callExpr(callee(functionDecl(parameterCountIs(1),
- hasName("strlen"))),
- hasArgument(0, stringLiteral().bind(
- "strlen_arg"))))));
- };
-
- // Match a string variable and a call to length() or size().
- const auto HasStringVariableAndSizeCallArgs = [](const auto StringArgIndex,
- const auto LengthArgIndex) {
- return allOf(
- hasArgument(StringArgIndex, declRefExpr(hasDeclaration(
- decl().bind("string_var_decl")))),
- hasArgument(LengthArgIndex,
- cxxMemberCallExpr(
- callee(cxxMethodDecl(isConst(), parameterCountIs(0),
- hasAnyName("size", "length"))),
- on(declRefExpr(
- to(decl(equalsBoundNode("string_var_decl"))))))));
- };
-
- // Match either one of the two cases above.
- const auto HasStringAndLengthArgs =
- [HasStringLiteralAndLengthArgs, HasStringVariableAndSizeCallArgs](
- const auto StringArgIndex, const auto LengthArgIndex) {
- return anyOf(
- HasStringLiteralAndLengthArgs(StringArgIndex, LengthArgIndex),
- HasStringVariableAndSizeCallArgs(StringArgIndex, LengthArgIndex));
- };
+ OnClassWithStartsWithFunction, hasArgument(0, expr().bind("needle")));
+ // Case 3: X.compare(0, LEN(Y), Y) [!=]= 0 -> starts_with.
const auto CompareExpr = cxxMemberCallExpr(
- // A method call with three arguments...
- argumentCountIs(3),
- // ... where the first argument is zero...
- hasArgument(0, ZeroLiteral),
- // ... named compare...
+ argumentCountIs(3), hasArgument(0, ZeroLiteral),
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 a length.
- HasStringAndLengthArgs(2, 1),
- // Bind search expression.
- hasArgument(2, expr().bind("search_expr")));
+ OnClassWithStartsWithFunction, hasArgument(2, expr().bind("needle")),
+ hasArgument(1, lengthExprForStringNode("needle")));
+ // Case 4: X.compare(LEN(X) - LEN(Y), LEN(Y), Y) [!=]= 0 -> ends_with.
+ const auto CompareEndsWithExpr = cxxMemberCallExpr(
+ argumentCountIs(3),
+ callee(cxxMethodDecl(hasName("compare")).bind("find_fun")),
+ OnClassWithEndsWithFunction, hasArgument(2, expr().bind("needle")),
+ hasArgument(1, lengthExprForStringNode("needle")),
+ hasArgument(0,
+ binaryOperator(hasOperatorName("-"),
+ hasLHS(lengthExprForStringNode("haystack")),
+ hasRHS(lengthExprForStringNode("needle")))));
+
+ // All cases comparing to 0.
Finder->addMatcher(
- // Match [=!]= with a zero on one side and (r?)find|compare on the other.
binaryOperator(
hasAnyOperatorName("==", "!="),
- hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr))
+ hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr,
+ CompareEndsWithExpr))
.bind("find_expr"),
ZeroLiteral))
.bind("expr"),
this);
+
+ // Case 5: X.rfind(Y) [!=]= LEN(X) - LEN(Y) -> ends_with.
+ Finder->addMatcher(
+ binaryOperator(
+ hasAnyOperatorName("==", "!="),
+ hasOperands(
+ cxxMemberCallExpr(
+ anyOf(
+ argumentCountIs(1),
+ allOf(argumentCountIs(2),
+ hasArgument(
+ 1,
+ anyOf(declRefExpr(to(varDecl(hasName("npos")))),
+ memberExpr(member(hasName("npos"))))))),
+ callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
+ OnClassWithEndsWithFunction,
+ hasArgument(0, expr().bind("needle")))
+ .bind("find_expr"),
+ binaryOperator(hasOperatorName("-"),
+ hasLHS(lengthExprForStringNode("haystack")),
+ hasRHS(lengthExprForStringNode("needle")))))
+ .bind("expr"),
+ this);
}
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 *SearchExpr = Result.Nodes.getNodeAs<Expr>("needle");
const auto *StartsWithFunction =
Result.Nodes.getNodeAs<CXXMethodDecl>("starts_with_fun");
-
- const auto *StringLiteralArg =
- Result.Nodes.getNodeAs<StringLiteral>("string_literal_arg");
- const auto *IntegerLiteralSizeArg =
- Result.Nodes.getNodeAs<IntegerLiteral>("integer_literal_size_arg");
- const auto *StrlenArg = Result.Nodes.getNodeAs<StringLiteral>("strlen_arg");
-
- // Filter out compare cases where the length does not match string literal.
- if (StringLiteralArg && IntegerLiteralSizeArg &&
- StringLiteralArg->getLength() !=
- IntegerLiteralSizeArg->getValue().getZExtValue()) {
- return;
- }
-
- if (StringLiteralArg && StrlenArg &&
- StringLiteralArg->getLength() != StrlenArg->getLength()) {
- return;
- }
+ const auto *EndsWithFunction =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("ends_with_fun");
+ assert(bool(StartsWithFunction) != bool(EndsWithFunction));
+ const CXXMethodDecl *ReplacementFunction =
+ StartsWithFunction ? StartsWithFunction : EndsWithFunction;
if (ComparisonExpr->getBeginLoc().isMacroID()) {
return;
@@ -154,9 +198,9 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
auto Diagnostic =
diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0")
- << StartsWithFunction->getName() << FindFun->getName() << Neg;
+ << ReplacementFunction->getName() << FindFun->getName() << Neg;
- // Remove possible arguments after search expression and ' [!=]= 0' suffix.
+ // Remove possible arguments after search expression and ' [!=]= .+' suffix.
Diagnostic << FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(
Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
@@ -164,16 +208,16 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
ComparisonExpr->getEndLoc()),
")");
- // Remove possible '0 [!=]= ' prefix.
+ // Remove possible '.+ [!=]= ' prefix.
Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc()));
- // Replace method name by 'starts_with'.
+ // Replace method name by '(starts|ends)_with'.
// Remove possible arguments before search expression.
Diagnostic << FixItHint::CreateReplacement(
CharSourceRange::getCharRange(FindExpr->getExprLoc(),
SearchExpr->getBeginLoc()),
- (StartsWithFunction->getName() + "(").str());
+ (ReplacementFunction->getName() + "(").str());
// 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 840191f321493f..17c2999bda84cd 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
@@ -14,7 +14,7 @@
namespace clang::tidy::modernize {
/// Checks for common roundabout ways to express ``starts_with`` and
-/// ``ends_with`` and suggests replacing with ``starts_with`` when the method is
+/// ``ends_with`` and suggests replacing with the simpler method when it is
/// available. Notably, this will work with ``std::string`` and
/// ``std::string_view``.
///
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 1b025e8f90f7ba..0f2640c6d45346 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,10 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`modernize-use-starts-ends-with
+ <clang-tidy/checks/modernize/use-starts-ends-with>` check to handle two cases
+ that can be replaced with ``ends_with``.
+
- Improved :doc:`modernize-use-std-format
<clang-tidy/checks/modernize/use-std-format>` check to support replacing
member function calls too.
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 34237ede30a30b..721e927e29849f 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
@@ -4,8 +4,8 @@ modernize-use-starts-ends-with
==============================
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``.
+and suggests replacing with the simpler method when it is available. Notably,
+this will work with ``std::string`` and ``std::string_view``.
.. code-block:: c++
@@ -13,6 +13,12 @@ Notably, this will work with ``std::string`` and ``std::string_view``.
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 */ }
+ if (s.compare(s.size() - strlen("suffix"), strlen("suffix"), "suffix") == 0) {
+ /* do something */
+ }
+ if (s.rfind("suffix") == (s.length() - 6)) {
+ /* do something */
+ }
becomes
@@ -22,3 +28,5 @@ becomes
if (s.starts_with("prefix")) { /* do something */ }
if (s.starts_with("prefix")) { /* do something */ }
if (s.starts_with("prefix")) { /* do something */ }
+ if (s.ends_with("suffix")) { /* do something */ }
+ if (s.ends_with("suffix")) { /* 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 0c160bc182b6eb..a0e8ebbb267cd0 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -64,6 +64,10 @@ struct basic_string {
constexpr bool starts_with(C ch) const noexcept;
constexpr bool starts_with(const C* s) const;
+ constexpr bool ends_with(std::basic_string_view<C, T> sv) const noexcept;
+ constexpr bool ends_with(C ch) const noexcept;
+ constexpr bool ends_with(const C* s) const;
+
_Type& operator[](size_type);
const _Type& operator[](size_type) const;
@@ -108,6 +112,10 @@ struct basic_string_view {
constexpr bool starts_with(C ch) const noexcept;
constexpr bool starts_with(const C* s) const;
+ constexpr bool ends_with(basic_string_view sv) const noexcept;
+ constexpr bool ends_with(C ch) const noexcept;
+ constexpr bool ends_with(const C* s) const;
+
constexpr int compare(basic_string_view sv) const noexcept;
static constexpr size_t npos = -1;
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 c5b2c86befd1fe..798af260a3b66c 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
@@ -208,6 +208,62 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
// CHECK-FIXES: !s.starts_with(sv);
+ s.compare(s.size() - 6, 6, "suffix") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ s.compare(s.size() - 6, strlen("abcdef"), "suffix") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ std::string suffix = "suffix";
+ s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind("suffix") == s.size() - 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ s.rfind("suffix") == s.size() - strlen("suffix");
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ s.rfind(suffix) == s.size() - suffix.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind(suffix, std::string::npos) == s.size() - suffix.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind(suffix) == (s.size() - suffix.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind(suffix, s.npos) == (s.size() - suffix.size());
+ // CHECK-...
[truncated]
|
|
@llvm/pr-subscribers-clang-tidy Author: Nicolas van Kempen (nicovank) ChangesAdd support for the following two patterns which are found occasionally, that can be replaced with haystack.compare(haystack.length() - needle.length(), needle.length(), needle) == 0;
haystack.rfind(needle) == (haystack.size() - needle.size());A big change is adding a Patch is 21.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105366.diff 6 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
index 89ee45faecd7f3..5eb3267adb0799 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -8,6 +8,7 @@
#include "UseStartsEndsWithCheck.h"
+#include "../utils/ASTUtils.h"
#include "../utils/OptionsUtils.h"
#include "clang/Lex/Lexer.h"
@@ -16,6 +17,63 @@
using namespace clang::ast_matchers;
namespace clang::tidy::modernize {
+struct NotLengthExprForStringNode {
+ NotLengthExprForStringNode(std::string ID, DynTypedNode Node,
+ ASTContext *Context)
+ : ID(std::move(ID)), Node(std::move(Node)), Context(Context) {}
+ bool operator()(const internal::BoundNodesMap &Nodes) const {
+ // Match a string literal and an integer size or strlen() call.
+ if (const auto *StringLiteralNode = Nodes.getNodeAs<StringLiteral>(ID)) {
+ if (const auto *IntegerLiteralSizeNode = Node.get<IntegerLiteral>()) {
+ return StringLiteralNode->getLength() !=
+ IntegerLiteralSizeNode->getValue().getZExtValue();
+ }
+
+ if (const auto *StrlenNode = Node.get<CallExpr>()) {
+ if (StrlenNode->getDirectCallee()->getName() != "strlen" ||
+ StrlenNode->getNumArgs() != 1) {
+ return true;
+ }
+
+ if (const auto *StrlenArgNode = dyn_cast<StringLiteral>(
+ StrlenNode->getArg(0)->IgnoreParenImpCasts())) {
+ return StrlenArgNode->getLength() != StringLiteralNode->getLength();
+ }
+ }
+ }
+
+ // Match a string variable and a call to length() or size().
+ if (const auto *ExprNode = Nodes.getNodeAs<Expr>(ID)) {
+ if (const auto *MemberCallNode = Node.get<CXXMemberCallExpr>()) {
+ const CXXMethodDecl *MethodDeclNode = MemberCallNode->getMethodDecl();
+ const StringRef Name = MethodDeclNode->getName();
+ if (!MethodDeclNode->isConst() || MethodDeclNode->getNumParams() != 0 ||
+ (Name != "size" && Name != "length")) {
+ return true;
+ }
+
+ if (const auto *OnNode =
+ dyn_cast<Expr>(MemberCallNode->getImplicitObjectArgument())) {
+ return !utils::areStatementsIdentical(OnNode->IgnoreParenImpCasts(),
+ ExprNode->IgnoreParenImpCasts(),
+ *Context);
+ }
+ }
+ }
+
+ return true;
+ }
+
+private:
+ std::string ID;
+ DynTypedNode Node;
+ ASTContext *Context;
+};
+
+AST_MATCHER_P(Expr, lengthExprForStringNode, std::string, ID) {
+ return Builder->removeBindings(NotLengthExprForStringNode(
+ ID, DynTypedNode::create(Node), &(Finder->getASTContext())));
+}
UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
ClangTidyContext *Context)
@@ -23,6 +81,7 @@ UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
const auto ZeroLiteral = integerLiteral(equals(0));
+
const auto HasStartsWithMethodWithName = [](const std::string &Name) {
return hasMethod(
cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
@@ -32,119 +91,104 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) {
anyOf(HasStartsWithMethodWithName("starts_with"),
HasStartsWithMethodWithName("startsWith"),
HasStartsWithMethodWithName("startswith"));
- const auto ClassWithStartsWithFunction = cxxRecordDecl(anyOf(
- HasStartsWithMethod, hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
- cxxRecordDecl(HasStartsWithMethod)))))));
+ const auto OnClassWithStartsWithFunction =
+ on(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+ anyOf(HasStartsWithMethod,
+ hasAnyBase(hasType(hasCanonicalType(
+ hasDeclaration(cxxRecordDecl(HasStartsWithMethod)))))))))));
+ const auto HasEndsWithMethodWithName = [](const std::string &Name) {
+ return hasMethod(
+ cxxMethodDecl(hasName(Name), isConst(), parameterCountIs(1))
+ .bind("ends_with_fun"));
+ };
+ const auto HasEndsWithMethod = anyOf(HasEndsWithMethodWithName("ends_with"),
+ HasEndsWithMethodWithName("endsWith"),
+ HasEndsWithMethodWithName("endswith"));
+ const auto OnClassWithEndsWithFunction =
+ on(expr(hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl(
+ anyOf(HasEndsWithMethod,
+ hasAnyBase(hasType(hasCanonicalType(hasDeclaration(
+ cxxRecordDecl(HasEndsWithMethod)))))))))))
+ .bind("haystack"));
+
+ // Case 1: X.find(Y) [!=]= 0 -> starts_with.
const auto FindExpr = cxxMemberCallExpr(
- // A method call with no second argument or the second argument is zero...
anyOf(argumentCountIs(1), hasArgument(1, ZeroLiteral)),
- // ... named find...
callee(cxxMethodDecl(hasName("find")).bind("find_fun")),
- // ... on a class with a starts_with function.
- on(hasType(
- hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
- // Bind search expression.
- hasArgument(0, expr().bind("search_expr")));
+ OnClassWithStartsWithFunction, hasArgument(0, expr().bind("needle")));
+ // Case 2: X.rfind(Y, 0) [!=]= 0 -> starts_with.
const auto RFindExpr = cxxMemberCallExpr(
- // A method call with a second argument of zero...
hasArgument(1, ZeroLiteral),
- // ... named rfind...
callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
- // ... on a class with a starts_with function.
- on(hasType(
- hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
- // Bind search expression.
- hasArgument(0, expr().bind("search_expr")));
-
- // Match a string literal and an integer or strlen() call matching the length.
- const auto HasStringLiteralAndLengthArgs = [](const auto StringArgIndex,
- const auto LengthArgIndex) {
- return allOf(
- hasArgument(StringArgIndex, stringLiteral().bind("string_literal_arg")),
- hasArgument(LengthArgIndex,
- anyOf(integerLiteral().bind("integer_literal_size_arg"),
- callExpr(callee(functionDecl(parameterCountIs(1),
- hasName("strlen"))),
- hasArgument(0, stringLiteral().bind(
- "strlen_arg"))))));
- };
-
- // Match a string variable and a call to length() or size().
- const auto HasStringVariableAndSizeCallArgs = [](const auto StringArgIndex,
- const auto LengthArgIndex) {
- return allOf(
- hasArgument(StringArgIndex, declRefExpr(hasDeclaration(
- decl().bind("string_var_decl")))),
- hasArgument(LengthArgIndex,
- cxxMemberCallExpr(
- callee(cxxMethodDecl(isConst(), parameterCountIs(0),
- hasAnyName("size", "length"))),
- on(declRefExpr(
- to(decl(equalsBoundNode("string_var_decl"))))))));
- };
-
- // Match either one of the two cases above.
- const auto HasStringAndLengthArgs =
- [HasStringLiteralAndLengthArgs, HasStringVariableAndSizeCallArgs](
- const auto StringArgIndex, const auto LengthArgIndex) {
- return anyOf(
- HasStringLiteralAndLengthArgs(StringArgIndex, LengthArgIndex),
- HasStringVariableAndSizeCallArgs(StringArgIndex, LengthArgIndex));
- };
+ OnClassWithStartsWithFunction, hasArgument(0, expr().bind("needle")));
+ // Case 3: X.compare(0, LEN(Y), Y) [!=]= 0 -> starts_with.
const auto CompareExpr = cxxMemberCallExpr(
- // A method call with three arguments...
- argumentCountIs(3),
- // ... where the first argument is zero...
- hasArgument(0, ZeroLiteral),
- // ... named compare...
+ argumentCountIs(3), hasArgument(0, ZeroLiteral),
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 a length.
- HasStringAndLengthArgs(2, 1),
- // Bind search expression.
- hasArgument(2, expr().bind("search_expr")));
+ OnClassWithStartsWithFunction, hasArgument(2, expr().bind("needle")),
+ hasArgument(1, lengthExprForStringNode("needle")));
+ // Case 4: X.compare(LEN(X) - LEN(Y), LEN(Y), Y) [!=]= 0 -> ends_with.
+ const auto CompareEndsWithExpr = cxxMemberCallExpr(
+ argumentCountIs(3),
+ callee(cxxMethodDecl(hasName("compare")).bind("find_fun")),
+ OnClassWithEndsWithFunction, hasArgument(2, expr().bind("needle")),
+ hasArgument(1, lengthExprForStringNode("needle")),
+ hasArgument(0,
+ binaryOperator(hasOperatorName("-"),
+ hasLHS(lengthExprForStringNode("haystack")),
+ hasRHS(lengthExprForStringNode("needle")))));
+
+ // All cases comparing to 0.
Finder->addMatcher(
- // Match [=!]= with a zero on one side and (r?)find|compare on the other.
binaryOperator(
hasAnyOperatorName("==", "!="),
- hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr))
+ hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr,
+ CompareEndsWithExpr))
.bind("find_expr"),
ZeroLiteral))
.bind("expr"),
this);
+
+ // Case 5: X.rfind(Y) [!=]= LEN(X) - LEN(Y) -> ends_with.
+ Finder->addMatcher(
+ binaryOperator(
+ hasAnyOperatorName("==", "!="),
+ hasOperands(
+ cxxMemberCallExpr(
+ anyOf(
+ argumentCountIs(1),
+ allOf(argumentCountIs(2),
+ hasArgument(
+ 1,
+ anyOf(declRefExpr(to(varDecl(hasName("npos")))),
+ memberExpr(member(hasName("npos"))))))),
+ callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")),
+ OnClassWithEndsWithFunction,
+ hasArgument(0, expr().bind("needle")))
+ .bind("find_expr"),
+ binaryOperator(hasOperatorName("-"),
+ hasLHS(lengthExprForStringNode("haystack")),
+ hasRHS(lengthExprForStringNode("needle")))))
+ .bind("expr"),
+ this);
}
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 *SearchExpr = Result.Nodes.getNodeAs<Expr>("needle");
const auto *StartsWithFunction =
Result.Nodes.getNodeAs<CXXMethodDecl>("starts_with_fun");
-
- const auto *StringLiteralArg =
- Result.Nodes.getNodeAs<StringLiteral>("string_literal_arg");
- const auto *IntegerLiteralSizeArg =
- Result.Nodes.getNodeAs<IntegerLiteral>("integer_literal_size_arg");
- const auto *StrlenArg = Result.Nodes.getNodeAs<StringLiteral>("strlen_arg");
-
- // Filter out compare cases where the length does not match string literal.
- if (StringLiteralArg && IntegerLiteralSizeArg &&
- StringLiteralArg->getLength() !=
- IntegerLiteralSizeArg->getValue().getZExtValue()) {
- return;
- }
-
- if (StringLiteralArg && StrlenArg &&
- StringLiteralArg->getLength() != StrlenArg->getLength()) {
- return;
- }
+ const auto *EndsWithFunction =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("ends_with_fun");
+ assert(bool(StartsWithFunction) != bool(EndsWithFunction));
+ const CXXMethodDecl *ReplacementFunction =
+ StartsWithFunction ? StartsWithFunction : EndsWithFunction;
if (ComparisonExpr->getBeginLoc().isMacroID()) {
return;
@@ -154,9 +198,9 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
auto Diagnostic =
diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0")
- << StartsWithFunction->getName() << FindFun->getName() << Neg;
+ << ReplacementFunction->getName() << FindFun->getName() << Neg;
- // Remove possible arguments after search expression and ' [!=]= 0' suffix.
+ // Remove possible arguments after search expression and ' [!=]= .+' suffix.
Diagnostic << FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(
Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0,
@@ -164,16 +208,16 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
ComparisonExpr->getEndLoc()),
")");
- // Remove possible '0 [!=]= ' prefix.
+ // Remove possible '.+ [!=]= ' prefix.
Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc()));
- // Replace method name by 'starts_with'.
+ // Replace method name by '(starts|ends)_with'.
// Remove possible arguments before search expression.
Diagnostic << FixItHint::CreateReplacement(
CharSourceRange::getCharRange(FindExpr->getExprLoc(),
SearchExpr->getBeginLoc()),
- (StartsWithFunction->getName() + "(").str());
+ (ReplacementFunction->getName() + "(").str());
// 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 840191f321493f..17c2999bda84cd 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h
@@ -14,7 +14,7 @@
namespace clang::tidy::modernize {
/// Checks for common roundabout ways to express ``starts_with`` and
-/// ``ends_with`` and suggests replacing with ``starts_with`` when the method is
+/// ``ends_with`` and suggests replacing with the simpler method when it is
/// available. Notably, this will work with ``std::string`` and
/// ``std::string_view``.
///
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 1b025e8f90f7ba..0f2640c6d45346 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,10 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`modernize-use-starts-ends-with
+ <clang-tidy/checks/modernize/use-starts-ends-with>` check to handle two cases
+ that can be replaced with ``ends_with``.
+
- Improved :doc:`modernize-use-std-format
<clang-tidy/checks/modernize/use-std-format>` check to support replacing
member function calls too.
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 34237ede30a30b..721e927e29849f 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
@@ -4,8 +4,8 @@ modernize-use-starts-ends-with
==============================
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``.
+and suggests replacing with the simpler method when it is available. Notably,
+this will work with ``std::string`` and ``std::string_view``.
.. code-block:: c++
@@ -13,6 +13,12 @@ Notably, this will work with ``std::string`` and ``std::string_view``.
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 */ }
+ if (s.compare(s.size() - strlen("suffix"), strlen("suffix"), "suffix") == 0) {
+ /* do something */
+ }
+ if (s.rfind("suffix") == (s.length() - 6)) {
+ /* do something */
+ }
becomes
@@ -22,3 +28,5 @@ becomes
if (s.starts_with("prefix")) { /* do something */ }
if (s.starts_with("prefix")) { /* do something */ }
if (s.starts_with("prefix")) { /* do something */ }
+ if (s.ends_with("suffix")) { /* do something */ }
+ if (s.ends_with("suffix")) { /* 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 0c160bc182b6eb..a0e8ebbb267cd0 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -64,6 +64,10 @@ struct basic_string {
constexpr bool starts_with(C ch) const noexcept;
constexpr bool starts_with(const C* s) const;
+ constexpr bool ends_with(std::basic_string_view<C, T> sv) const noexcept;
+ constexpr bool ends_with(C ch) const noexcept;
+ constexpr bool ends_with(const C* s) const;
+
_Type& operator[](size_type);
const _Type& operator[](size_type) const;
@@ -108,6 +112,10 @@ struct basic_string_view {
constexpr bool starts_with(C ch) const noexcept;
constexpr bool starts_with(const C* s) const;
+ constexpr bool ends_with(basic_string_view sv) const noexcept;
+ constexpr bool ends_with(C ch) const noexcept;
+ constexpr bool ends_with(const C* s) const;
+
constexpr int compare(basic_string_view sv) const noexcept;
static constexpr size_t npos = -1;
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 c5b2c86befd1fe..798af260a3b66c 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
@@ -208,6 +208,62 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss,
// CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with
// CHECK-FIXES: !s.starts_with(sv);
+ s.compare(s.size() - 6, 6, "suffix") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ s.compare(s.size() - 6, strlen("abcdef"), "suffix") == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ std::string suffix = "suffix";
+ s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind("suffix") == s.size() - 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ s.rfind("suffix") == s.size() - strlen("suffix");
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with("suffix");
+
+ s.rfind(suffix) == s.size() - suffix.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind(suffix, std::string::npos) == s.size() - suffix.size();
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind(suffix) == (s.size() - suffix.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use ends_with
+ // CHECK-FIXES: s.ends_with(suffix);
+
+ s.rfind(suffix, s.npos) == (s.size() - suffix.size());
+ // CHECK-...
[truncated]
|
EugeneZelenko
requested review from
PiotrZSL,
5chmidti,
SimplyDanny and
njames93
and removed request for
PiotrZSL
…_with patterns Add support for the following two patterns: ``` haystack.compare(haystack.length() - needle.length(), needle.length(), needle) == 0; haystack.rfind(needle) == (haystack.size() - needle.size()); ```
|
Rebase to resolve merge conflict. Ping. |
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.
Add support for the following two patterns which are found occasionally, that can be replaced with
ends_with.haystack.compare(haystack.length() - needle.length(), needle.length(), needle) == 0; haystack.rfind(needle) == (haystack.size() - needle.size());A big change is adding a
lengthExprForStringNodecustom matcher which makes it easier to express an expression representing the length of a previously bound string node.