From fd8fc5e8d93849f4a2c8dea087690b1a0e6ea7df Mon Sep 17 00:00:00 2001 From: Ivan Gerasimov Date: Wed, 22 Dec 2021 16:33:09 +0100 Subject: [PATCH] [clang-tidy] abseil-string-find-startswith: detect `s.rfind(z, 0) == 0` Suggest converting `std::string::rfind()` calls to `absl::StartsWith()` where possible. --- .../abseil/StringFindStartswithCheck.cpp | 35 ++++++++++++++++--- .../checks/abseil-string-find-startswith.rst | 8 +++-- .../checkers/abseil-string-find-startswith.cpp | 40 ++++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp b/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp index 5741c0d..e834c8a 100644 --- a/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp +++ b/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp @@ -40,7 +40,7 @@ void StringFindStartswithCheck::registerMatchers(MatchFinder *Finder) { auto StringFind = cxxMemberCallExpr( // .find()-call on a string... - callee(cxxMethodDecl(hasName("find"))), + callee(cxxMethodDecl(hasName("find")).bind("findfun")), on(hasType(StringType)), // ... with some search expression ... hasArgument(0, expr().bind("needle")), @@ -55,6 +55,25 @@ void StringFindStartswithCheck::registerMatchers(MatchFinder *Finder) { ignoringParenImpCasts(StringFind.bind("findexpr")))) .bind("expr"), this); + + auto StringRFind = cxxMemberCallExpr( + // .rfind()-call on a string... + callee(cxxMethodDecl(hasName("rfind")).bind("findfun")), + on(hasType(StringType)), + // ... with some search expression ... + hasArgument(0, expr().bind("needle")), + // ... and "0" as second argument. + hasArgument(1, ZeroLiteral)); + + Finder->addMatcher( + // Match [=!]= with either a zero or npos on one side and a string.rfind + // on the other. + binaryOperator( + hasAnyOperatorName("==", "!="), + hasOperands(ignoringParenImpCasts(ZeroLiteral), + ignoringParenImpCasts(StringRFind.bind("findexpr")))) + .bind("expr"), + this); } void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) { @@ -69,6 +88,11 @@ void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) { const Expr *Haystack = Result.Nodes.getNodeAs("findexpr") ->getImplicitObjectArgument(); assert(Haystack != nullptr); + const CXXMethodDecl *FindFun = + Result.Nodes.getNodeAs("findfun"); + assert(FindFun != nullptr); + + bool Rev = FindFun->getName().contains("rfind"); if (ComparisonExpr->getBeginLoc().isMacroID()) return; @@ -86,10 +110,11 @@ void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) { bool Neg = ComparisonExpr->getOpcode() == BO_NE; // Create the warning message and a FixIt hint replacing the original expr. - auto Diagnostic = diag(ComparisonExpr->getBeginLoc(), - "use %select{absl::StartsWith|!absl::StartsWith}0 " - "instead of find() %select{==|!=}0 0") - << Neg; + auto Diagnostic = + diag(ComparisonExpr->getBeginLoc(), + "use %select{absl::StartsWith|!absl::StartsWith}0 " + "instead of %select{find()|rfind()}1 %select{==|!=}0 0") + << Neg << Rev; Diagnostic << FixItHint::CreateReplacement( ComparisonExpr->getSourceRange(), diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-startswith.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-startswith.rst index 43f35ac..8224c37 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-startswith.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-startswith.rst @@ -3,14 +3,15 @@ abseil-string-find-startswith ============================= -Checks whether a ``std::string::find()`` result is compared with 0, and -suggests replacing with ``absl::StartsWith()``. This is both a readability and -performance issue. +Checks whether a ``std::string::find()`` or ``std::string::rfind()`` result is +compared with 0, and suggests replacing with ``absl::StartsWith()``. This is +both a readability and performance issue. .. code-block:: c++ string s = "..."; if (s.find("Hello World") == 0) { /* do something */ } + if (s.rfind("Hello World", 0) == 0) { /* do something */ } becomes @@ -19,6 +20,7 @@ becomes string s = "..."; if (absl::StartsWith(s, "Hello World")) { /* do something */ } + if (absl::StartsWith(s, "Hello World")) { /* do something */ } Options diff --git a/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp b/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp index 89365bf..7a1fd82 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/abseil-string-find-startswith.cpp @@ -1,6 +1,8 @@ // RUN: %check_clang_tidy %s abseil-string-find-startswith %t -- \ // RUN: -config="{CheckOptions: [{key: 'abseil-string-find-startswith.StringLikeClasses', value: '::std::basic_string;::basic_string'}]}" +using size_t = decltype(sizeof(int)); + namespace std { template class allocator {}; template class char_traits {}; @@ -13,12 +15,17 @@ struct basic_string { ~basic_string(); int find(basic_string s, int pos = 0); int find(const char *s, int pos = 0); + int rfind(basic_string s, int pos = npos); + int rfind(const char *s, int pos = npos); + static constexpr size_t npos = -1; }; typedef basic_string string; typedef basic_string wstring; struct cxx_string { int find(const char *s, int pos = 0); + int rfind(const char *s, int pos = npos); + static constexpr size_t npos = -1; }; } // namespace std @@ -61,9 +68,42 @@ void tests(std::string s, global_string s2) { // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s2, "a");{{$}} + s.rfind("a", 0) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith instead of rfind() == 0 [abseil-string-find-startswith] + // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, "a");{{$}} + + s.rfind(s, 0) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith + // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s, s);{{$}} + + s.rfind("aaa", 0) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith + // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "aaa");{{$}} + + s.rfind(foo(foo(bar())), 0) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith + // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, foo(foo(bar())));{{$}} + + if (s.rfind("....", 0) == 0) { /* do something */ } + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use absl::StartsWith + // CHECK-FIXES: {{^[[:space:]]*}}if (absl::StartsWith(s, "....")) { /* do something */ }{{$}} + + 0 != s.rfind("a", 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use !absl::StartsWith + // CHECK-FIXES: {{^[[:space:]]*}}!absl::StartsWith(s, "a");{{$}} + + s2.rfind("a", 0) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use absl::StartsWith + // CHECK-FIXES: {{^[[:space:]]*}}absl::StartsWith(s2, "a");{{$}} + // expressions that don't trigger the check are here. A_MACRO(s.find("a"), 0); + A_MACRO(s.rfind("a", 0), 0); s.find("a", 1) == 0; s.find("a", 1) == 1; s.find("a") == 1; + s.rfind("a", 1) == 0; + s.rfind("a", 1) == 1; + s.rfind("a") == 0; + s.rfind("a") == 1; } -- 2.7.4