[clang-tools-extra] [clang-tidy][modernize-use-starts-ends-with] Add support for two ends_with patterns (PR #110448)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Sep 29 20:51:38 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: Nicolas van Kempen (nicovank)
<details>
<summary>Changes</summary>
Add support for the following two patterns:
```
haystack.compare(haystack.length() - needle.length(), needle.length(), needle) == 0;
haystack.rfind(needle) == (haystack.size() - needle.size());
```
---
Patch is 21.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110448.diff
6 Files Affected:
- (modified) clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp (+135-91)
- (modified) clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h (+1-1)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
- (modified) clang-tools-extra/docs/clang-tidy/checks/modernize/use-starts-ends-with.rst (+10-2)
- (modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string (+8)
- (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-starts-ends-with.cpp (+57)
``````````diff
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 7d37a4b03222cf..5bcca66df13b0e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -161,6 +161,10 @@ Changes in existing checks
as a replacement for parameters of incomplete C array type in C++20 and
``std::array`` or ``std::vector`` before C++20.
+- 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(s...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/110448
More information about the cfe-commits
mailing list