https://github.com/nicovank created https://github.com/llvm/llvm-project/pull/72385
Matchers are copied over from abseil-string-find-startswith, only the error message is different and suggests `std::{string|string_view}::starts_with` instead of the Abseil equivalent. >From 5e5a2bac0f7373e6b1830fddc609e97dc61df9d4 Mon Sep 17 00:00:00 2001 From: Nicolas van Kempen <nvankem...@fb.com> Date: Wed, 15 Nov 2023 01:13:10 -0800 Subject: [PATCH] [clang-tidy] Add new modernize-string-find-startswith check Matchers are copied over from abseil-string-find-startswith, only the error message is different and suggests `std::{string|string_view}::starts_with` instead of the Abseil equivalent. --- .../abseil/StringFindStartswithCheck.h | 5 +- .../clang-tidy/modernize/CMakeLists.txt | 1 + .../modernize/ModernizeTidyModule.cpp | 3 + .../modernize/StringFindStartswithCheck.cpp | 111 ++++++++++++++++++ .../modernize/StringFindStartswithCheck.h | 41 +++++++ clang-tools-extra/docs/ReleaseNotes.rst | 8 ++ .../checks/abseil/string-find-startswith.rst | 4 + .../docs/clang-tidy/checks/list.rst | 1 + .../modernize/string-find-startswith.rst | 32 +++++ .../abseil/string-find-startswith.cpp | 2 +- .../modernize/string-find-startswith.cpp | 77 ++++++++++++ 11 files changed, 283 insertions(+), 2 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/modernize/string-find-startswith.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/string-find-startswith.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.h b/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.h index 923b5caece5439b..5019b7000f1d062 100644 --- a/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.h +++ b/clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.h @@ -21,7 +21,6 @@ namespace clang::tidy::abseil { // Find string.find(...) == 0 comparisons and suggest replacing with StartsWith. // FIXME(niko): Add similar check for EndsWith -// FIXME(niko): Add equivalent modernize checks for C++20's std::starts_With class StringFindStartswithCheck : public ClangTidyCheck { public: using ClangTidyCheck::ClangTidyCheck; @@ -31,6 +30,10 @@ class StringFindStartswithCheck : public ClangTidyCheck { void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + // Prefer modernize-string-find-startswith when C++20 is available. + return LangOpts.CPlusPlus && !LangOpts.CPlusPlus20; + } private: const std::vector<StringRef> StringLikeClasses; diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index 717c400c4790330..541f58304119856 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -25,6 +25,7 @@ add_clang_library(clangTidyModernizeModule ReplaceRandomShuffleCheck.cpp ReturnBracedInitListCheck.cpp ShrinkToFitCheck.cpp + StringFindStartswithCheck.cpp TypeTraitsCheck.cpp UnaryStaticAssertCheck.cpp UseAutoCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index 73751cf2705068d..1fcbff79ddc6f96 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -26,6 +26,7 @@ #include "ReplaceRandomShuffleCheck.h" #include "ReturnBracedInitListCheck.h" #include "ShrinkToFitCheck.h" +#include "StringFindStartswithCheck.h" #include "TypeTraitsCheck.h" #include "UnaryStaticAssertCheck.h" #include "UseAutoCheck.h" @@ -66,6 +67,8 @@ class ModernizeModule : public ClangTidyModule { CheckFactories.registerCheck<MakeSharedCheck>("modernize-make-shared"); CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique"); CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value"); + CheckFactories.registerCheck<StringFindStartswithCheck>( + "modernize-string-find-startswith"); CheckFactories.registerCheck<UseStdPrintCheck>("modernize-use-std-print"); CheckFactories.registerCheck<RawStringLiteralCheck>( "modernize-raw-string-literal"); diff --git a/clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.cpp new file mode 100644 index 000000000000000..7b992d77d0e7bb5 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.cpp @@ -0,0 +1,111 @@ +//===--- StringFindStartswithCheck.cpp - clang-tidy -----------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "StringFindStartswithCheck.h" + +#include "../utils/OptionsUtils.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::modernize { + +const auto DefaultStringLikeClasses = + "::std::basic_string;::std::basic_string_view"; + +StringFindStartswithCheck::StringFindStartswithCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + StringLikeClasses(utils::options::parseStringList( + Options.get("StringLikeClasses", DefaultStringLikeClasses))) {} + +void StringFindStartswithCheck::registerMatchers(MatchFinder *Finder) { + const auto ZeroLiteral = integerLiteral(equals(0)); + const auto StringClassMatcher = cxxRecordDecl(hasAnyName(StringLikeClasses)); + const auto StringType = hasUnqualifiedDesugaredType( + recordType(hasDeclaration(StringClassMatcher))); + + const auto StringFind = cxxMemberCallExpr( + // .find()-call on a string... + callee(cxxMethodDecl(hasName("find")).bind("findfun")), + on(hasType(StringType)), + // ... with some search expression ... + hasArgument(0, expr().bind("needle")), + // ... and either "0" as second argument or the default argument (also 0). + anyOf(hasArgument(1, ZeroLiteral), hasArgument(1, cxxDefaultArgExpr()))); + + Finder->addMatcher( + // Match [=!]= with a zero on one side and a string.find on the other. + binaryOperator( + hasAnyOperatorName("==", "!="), + hasOperands(ignoringParenImpCasts(ZeroLiteral), + ignoringParenImpCasts(StringFind.bind("findexpr")))) + .bind("expr"), + this); + + const 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 a zero 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) { + const auto &Context = *Result.Context; + const auto &Source = Context.getSourceManager(); + + const auto *ComparisonExpr = Result.Nodes.getNodeAs<BinaryOperator>("expr"); + const auto *Needle = Result.Nodes.getNodeAs<Expr>("needle"); + const auto *Haystack = Result.Nodes.getNodeAs<CXXMemberCallExpr>("findexpr") + ->getImplicitObjectArgument(); + const auto *FindFun = Result.Nodes.getNodeAs<CXXMethodDecl>("findfun"); + + if (ComparisonExpr->getBeginLoc().isMacroID()) { + return; + } + + const auto Rev = FindFun->getName().contains("rfind"); + const auto Neg = ComparisonExpr->getOpcode() == BO_NE; + + const auto NeedleExprCode = Lexer::getSourceText( + CharSourceRange::getTokenRange(Needle->getSourceRange()), Source, + Context.getLangOpts()); + const auto HaystackExprCode = Lexer::getSourceText( + CharSourceRange::getTokenRange(Haystack->getSourceRange()), Source, + Context.getLangOpts()); + + const auto ReplacementCode = (Neg ? "!" : "") + HaystackExprCode + + ".starts_with(" + NeedleExprCode + ")"; + + diag(ComparisonExpr->getBeginLoc(), + "use starts_with " + "instead of %select{find()|rfind()}0 %select{==|!=}1 0") + << Rev << Neg + << FixItHint::CreateReplacement(ComparisonExpr->getSourceRange(), + ReplacementCode.str()); +} + +void StringFindStartswithCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StringLikeClasses", + utils::options::serializeStringList(StringLikeClasses)); +} + +} // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.h b/clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.h new file mode 100644 index 000000000000000..fc12749de82cb3c --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/StringFindStartswithCheck.h @@ -0,0 +1,41 @@ +//===--- StringFindStartswithCheck.h - clang-tidy ---------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_STRINGFINDSTARTSWITHCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_STRINGFINDSTARTSWITHCHECK_H + +#include "../ClangTidyCheck.h" + +#include <vector> + +namespace clang::tidy::modernize { + +/// Checks whether a ``std::string::find()`` or ``std::string::rfind()`` (and +/// corresponding ``std::string_view`` methods) result is compared with 0, and +/// suggests replacing with ``starts_with()``. This is both a readability and a +/// performance issue. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/string-find-startswith.html +class StringFindStartswithCheck : public ClangTidyCheck { +public: + StringFindStartswithCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus20; + } + +private: + const std::vector<StringRef> StringLikeClasses; +}; + +} // namespace clang::tidy::modernize + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_STRINGFINDSTARTSWITHCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 353c6fe20269274..8f4ab856bf0a696 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -181,6 +181,14 @@ New checks points in a coroutine. Such hostile types include scoped-lockable types and types belonging to a configurable denylist. +- New :doc:`modernize-string-find-startswith + <clang-tidy/checks/modernize/string-find-startswith>` check. + + Checks whether a ``std::string::find()`` or ``std::string::rfind()`` (and + corresponding ``std::string_view`` methods) result is compared with 0, and + suggests replacing with ``starts_with()``. This is both a readability and a + performance issue. + - New :doc:`modernize-use-constraints <clang-tidy/checks/modernize/use-constraints>` check. 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 c82c38772a5c9a8..a22192a37f07818 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 @@ -8,6 +8,10 @@ corresponding ``std::string_view`` methods) result is compared with 0, and suggests replacing with ``absl::StartsWith()``. This is both a readability and performance issue. +``starts_with`` was added as a built-in function on those types in C++20. If +available, prefer enabling modernize-string-find-startswith instead of this +check. + .. code-block:: c++ string s = "..."; diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 6f987ba1672e3f2..ecf5c38a62d1a39 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -279,6 +279,7 @@ Clang-Tidy Checks :doc:`modernize-replace-random-shuffle <modernize/replace-random-shuffle>`, "Yes" :doc:`modernize-return-braced-init-list <modernize/return-braced-init-list>`, "Yes" :doc:`modernize-shrink-to-fit <modernize/shrink-to-fit>`, "Yes" + :doc:`modernize-string-find-startswith <modernize/string-find-startswith>`, "Yes" :doc:`modernize-type-traits <modernize/type-traits>`, "Yes" :doc:`modernize-unary-static-assert <modernize/unary-static-assert>`, "Yes" :doc:`modernize-use-auto <modernize/use-auto>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/string-find-startswith.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/string-find-startswith.rst new file mode 100644 index 000000000000000..997703dd59c3b3c --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/string-find-startswith.rst @@ -0,0 +1,32 @@ +.. title:: clang-tidy - modernize-string-find-startswith + +modernize-string-find-startswith +================================ + +Checks whether a ``std::string::find()`` or ``std::string::rfind()`` (and +corresponding ``std::string_view`` methods) result is compared with 0, and +suggests replacing with ``starts_with()``. This is both a readability and a +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 + +.. code-block:: c++ + + string s = "..."; + if (s.starts_with("Hello World")) { /* do something */ } + if (s.starts_with("Hello World")) { /* do something */ } + + +Options +------- + +.. option:: StringLikeClasses + + Semicolon-separated list of names of string-like classes. By default both + ``std::basic_string`` and ``std::basic_string_view`` are considered. 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 417598790bc007f..aabb30fe34f782c 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,4 +1,4 @@ -// RUN: %check_clang_tidy %s abseil-string-find-startswith %t -- \ +// RUN: %check_clang_tidy -std=c++17 %s abseil-string-find-startswith %t -- \ // RUN: -config="{CheckOptions: \ // RUN: {abseil-string-find-startswith.StringLikeClasses: \ // RUN: '::std::basic_string;::std::basic_string_view;::basic_string'}}" \ diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/string-find-startswith.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/string-find-startswith.cpp new file mode 100644 index 000000000000000..50e5c0ed93ea066 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/string-find-startswith.cpp @@ -0,0 +1,77 @@ +// RUN: %check_clang_tidy -std=c++20 %s modernize-string-find-startswith %t -- -- -isystem %clang_tidy_headers + +#include <string> + +std::string foo(std::string); +std::string bar(); + +#define A_MACRO(x, y) ((x) == (y)) + +void test(std::string s, std::string_view sv) { + s.find("a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of find() == 0 + // CHECK-FIXES: s.starts_with("a"); + + s.find(s) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(s); + + s.find("aaa") != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with("aaa"); + + s.find(foo(foo(bar()))) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with(foo(foo(bar()))); + + if (s.find("....") == 0) { /* do something */ } + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: if (s.starts_with("....")) + + 0 != s.find("a"); + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with("a"); + + s.rfind("a", 0) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of rfind() == 0 + // CHECK-FIXES: s.starts_with("a"); + + s.rfind(s, 0) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(s); + + s.rfind("aaa", 0) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with("aaa"); + + s.rfind(foo(foo(bar())), 0) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with(foo(foo(bar()))); + + if (s.rfind("....", 0) == 0) { /* do something */ } + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use starts_with + // CHECK-FIXES: if (s.starts_with("....")) + + 0 != s.rfind("a", 0); + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with("a"); + + sv.find("a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: sv.starts_with("a"); + + sv.rfind("a", 0) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !sv.starts_with("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; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits