diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp index 1231f954298ad..234f6790a7ed9 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp @@ -18,6 +18,17 @@ using namespace clang::ast_matchers; namespace clang::tidy::modernize { + +static bool isNegativeComparison(const Expr *ComparisonExpr) { + if (const auto *BO = llvm::dyn_cast(ComparisonExpr)) + return BO->getOpcode() == BO_NE; + + if (const auto *Op = llvm::dyn_cast(ComparisonExpr)) + return Op->getOperator() == OO_ExclaimEqual; + + return false; +} + struct NotLengthExprForStringNode { NotLengthExprForStringNode(std::string ID, DynTypedNode Node, ASTContext *Context) @@ -171,10 +182,26 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) { hasRHS(lengthExprForStringNode("needle"))))) .bind("expr"), this); + + // Case 6: X.substr(0, LEN(Y)) [!=]= Y -> starts_with. + Finder->addMatcher( + cxxOperatorCallExpr( + hasAnyOperatorName("==", "!="), + hasOperands( + expr().bind("needle"), + cxxMemberCallExpr( + argumentCountIs(2), hasArgument(0, ZeroLiteral), + hasArgument(1, lengthExprForStringNode("needle")), + callee(cxxMethodDecl(hasName("substr"), + ofClass(OnClassWithStartsWithFunction)) + .bind("find_fun"))) + .bind("find_expr"))) + .bind("expr"), + this); } void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) { - const auto *ComparisonExpr = Result.Nodes.getNodeAs("expr"); + const auto *ComparisonExpr = Result.Nodes.getNodeAs("expr"); const auto *FindExpr = Result.Nodes.getNodeAs("find_expr"); const auto *FindFun = Result.Nodes.getNodeAs("find_fun"); const auto *SearchExpr = Result.Nodes.getNodeAs("needle"); @@ -183,39 +210,42 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) { const auto *EndsWithFunction = Result.Nodes.getNodeAs("ends_with_fun"); assert(bool(StartsWithFunction) != bool(EndsWithFunction)); + const CXXMethodDecl *ReplacementFunction = StartsWithFunction ? StartsWithFunction : EndsWithFunction; - if (ComparisonExpr->getBeginLoc().isMacroID()) + if (ComparisonExpr->getBeginLoc().isMacroID() || + FindExpr->getBeginLoc().isMacroID()) return; - const bool Neg = ComparisonExpr->getOpcode() == BO_NE; + // Make sure FindExpr->getArg(0) can be used to make a range in the FitItHint. + if (FindExpr->getNumArgs() == 0) + return; - auto Diagnostic = - diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0") - << ReplacementFunction->getName() << FindFun->getName() << Neg; + // Retrieve the source text of the search expression. + const auto SearchExprText = Lexer::getSourceText( + CharSourceRange::getTokenRange(SearchExpr->getSourceRange()), + *Result.SourceManager, Result.Context->getLangOpts()); - // Remove possible arguments after search expression and ' [!=]= .+' suffix. - Diagnostic << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange( - Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0, - *Result.SourceManager, getLangOpts()), - ComparisonExpr->getEndLoc()), - ")"); + auto Diagnostic = diag(FindExpr->getExprLoc(), "use %0 instead of %1") + << ReplacementFunction->getName() << FindFun->getName(); - // Remove possible '.+ [!=]= ' prefix. + // Remove everything before the function call. Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange( ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc())); - // Replace method name by '(starts|ends)_with'. - // Remove possible arguments before search expression. + // Rename the function to `starts_with` or `ends_with`. + Diagnostic << FixItHint::CreateReplacement(FindExpr->getExprLoc(), + ReplacementFunction->getName()); + + // Replace arguments and everything after the function call. Diagnostic << FixItHint::CreateReplacement( - CharSourceRange::getCharRange(FindExpr->getExprLoc(), - SearchExpr->getBeginLoc()), - (ReplacementFunction->getName() + "(").str()); + CharSourceRange::getTokenRange(FindExpr->getArg(0)->getBeginLoc(), + ComparisonExpr->getEndLoc()), + (SearchExprText + ")").str()); - // Add possible negation '!'. - if (Neg) + // Add negation if necessary. + if (isNegativeComparison(ComparisonExpr)) Diagnostic << FixItHint::CreateInsertion(FindExpr->getBeginLoc(), "!"); } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index db971f08ca3db..da63724952b8e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -243,8 +243,9 @@ Changes in existing checks ``NULL``/``__null`` (but not ``0``) when used with a templated type. - Improved :doc:`modernize-use-starts-ends-with - ` check to handle two cases - that can be replaced with ``ends_with`` + ` check to handle two new + cases from ``rfind`` and ``compare`` to ``ends_with``, and one new case from + ``substr`` to ``starts_with``, and a small adjustment to the diagnostic message. - Improved :doc:`modernize-use-std-format ` check to support replacing 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 721e927e29849..78cd900885ac3 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 @@ -7,26 +7,16 @@ Checks for common roundabout ways to express ``starts_with`` and ``ends_with`` 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++ +Covered scenarios: - 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 */ } - if (s.compare(s.size() - strlen("suffix"), strlen("suffix"), "suffix") == 0) { - /* do something */ - } - if (s.rfind("suffix") == (s.length() - 6)) { - /* do something */ - } - -becomes - -.. code-block:: c++ - - std::string s = "..."; - 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 */ } +==================================================== ===================== +Expression Replacement +---------------------------------------------------- --------------------- +``u.find(v) == 0`` ``u.starts_with(v)`` +``u.rfind(v, 0) != 0`` ``!u.starts_with(v)`` +``u.compare(0, v.size(), v) == 0`` ``u.starts_with(v)`` +``u.substr(0, v.size()) == v`` ``u.starts_with(v)`` +``v != u.substr(0, v.size())`` ``!u.starts_with(v)`` +``u.compare(u.size() - v.size(), v.size(), v) == 0`` ``u.ends_with(v)`` +``u.rfind(v) == u.size() - v.size()`` ``u.ends_with(v)`` +==================================================== ===================== 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 a0e8ebbb267cd..51f6817416906 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string @@ -60,6 +60,8 @@ struct basic_string { _Type& insert(size_type pos, const C* s); _Type& insert(size_type pos, const C* s, size_type n); + _Type substr(size_type pos = 0, size_type count = npos) const; + constexpr bool starts_with(std::basic_string_view sv) const noexcept; constexpr bool starts_with(C ch) const noexcept; constexpr bool starts_with(const C* s) const; 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 91477241e82e5..18a65f7f1cf26 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 @@ -36,7 +36,7 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss, string_like sl, string_like_camel slc, prefer_underscore_version puv, prefer_underscore_version_flip puvf) { s.find("a") == 0; - // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of find() == 0 + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of find [modernize-use-starts-ends-with] // CHECK-FIXES: s.starts_with("a"); (((((s)).find("a")))) == ((0)); @@ -68,7 +68,7 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss, // 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-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of rfind [modernize-use-starts-ends-with] // CHECK-FIXES: s.starts_with("a"); s.rfind(s, 0) == 0; @@ -91,16 +91,6 @@ 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("a"); - #define STR(x) std::string(x) - 0 == STR(s).find("a"); - // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with - // CHECK-FIXES: STR(s).starts_with("a"); - - #define STRING s - if (0 == STRING.find("ala")) { /* do something */} - // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with - // CHECK-FIXES: if (STRING.starts_with("ala")) - #define FIND find s.FIND("a") == 0; // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with @@ -149,11 +139,11 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss, // CHECK-FIXES: puvf.starts_with("a"); s.compare(0, 1, "a") == 0; - // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare() == 0 + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare [modernize-use-starts-ends-with] // 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-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare [modernize-use-starts-ends-with] // CHECK-FIXES: !s.starts_with("a"); s.compare(0, strlen("a"), "a") == 0; @@ -265,4 +255,68 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss, s.compare(0, 1, "ab") == 0; s.rfind(suffix, 1) == s.size() - suffix.size(); + + #define STR(x) std::string(x) + 0 == STR(s).find("a"); + + #define STRING s + if (0 == STRING.find("ala")) { /* do something */} +} + +void test_substr() { + std::string str("hello world"); + std::string prefix = "hello"; + + // Basic pattern + str.substr(0, 5) == "hello"; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: str.starts_with("hello"); + + // With string literal on left side + "hello" == str.substr(0, 5); + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: str.starts_with("hello"); + + // Inequality comparison + str.substr(0, 5) != "world"; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: !str.starts_with("world"); + + // Ensure non-zero start position is not transformed + str.substr(1, 5) == "hello"; + str.substr(0, 4) == "hello"; // Length mismatch + + size_t len = 5; + str.substr(0, len) == "hello"; // Non-constant length + + // String literal with size calculation + str.substr(0, strlen("hello")) == "hello"; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: str.starts_with("hello"); + + str.substr(0, prefix.size()) == prefix; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: str.starts_with(prefix); + + str.substr(0, prefix.length()) == prefix; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: str.starts_with(prefix); + + // Tests to verify macro behavior + #define MSG "hello" + str.substr(0, strlen(MSG)) == MSG; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of substr [modernize-use-starts-ends-with] + // CHECK-FIXES: str.starts_with(MSG); + + #define STARTS_WITH(X, Y) (X).substr(0, (Y).size()) == (Y) + STARTS_WITH(str, prefix); + + #define SUBSTR(X, A, B) (X).substr((A), (B)) + SUBSTR(str, 0, 6) == "prefix"; + + #define STR() str + SUBSTR(STR(), 0, 6) == "prefix"; + "prefix" == SUBSTR(STR(), 0, 6); + + str.substr(0, strlen("hello123")) == "hello"; }