From ef5906989ae2004100ff56dc5ab59be2be9d5c99 Mon Sep 17 00:00:00 2001 From: Nicolas van Kempen Date: Tue, 23 Apr 2024 17:35:05 -0400 Subject: [clang-tidy][modernize-use-starts-ends-with] Add support for compare() (#89530) Using `compare` is the next most common roundabout way to express `starts_with` before it was added to the standard. In this case, using `starts_with` is a readability improvement. Extend existing `modernize-use-starts-ends-with` to cover this case. ``` // The following will now be replaced by starts_with(). string.compare(0, strlen("prefix"), "prefix") == 0; string.compare(0, 6, "prefix") == 0; string.compare(0, prefix.length(), prefix) == 0; string.compare(0, prefix.size(), prefix) == 0; ``` --- .../modernize/UseStartsEndsWithCheck.cpp | 103 ++++++++++++++++++--- .../clang-tidy/modernize/UseStartsEndsWithCheck.h | 7 +- clang-tools-extra/docs/ReleaseNotes.rst | 4 + .../checks/modernize/use-starts-ends-with.rst | 8 +- .../test/clang-tidy/checkers/Inputs/Headers/string | 4 + .../clang-tidy/checkers/Inputs/Headers/string.h | 1 + .../checkers/abseil/redundant-strcat-calls.cpp | 2 - .../checkers/modernize/use-starts-ends-with.cpp | 55 +++++++++++ 8 files changed, 162 insertions(+), 22 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp index 062f6e9..89ee45f 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp @@ -43,7 +43,9 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) { callee(cxxMethodDecl(hasName("find")).bind("find_fun")), // ... on a class with a starts_with function. on(hasType( - hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction))))); + hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))), + // Bind search expression. + hasArgument(0, expr().bind("search_expr"))); const auto RFindExpr = cxxMemberCallExpr( // A method call with a second argument of zero... @@ -52,15 +54,68 @@ void UseStartsEndsWithCheck::registerMatchers(MatchFinder *Finder) { callee(cxxMethodDecl(hasName("rfind")).bind("find_fun")), // ... on a class with a starts_with function. on(hasType( - hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction))))); + 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")))))))); + }; - const auto FindOrRFindExpr = - cxxMemberCallExpr(anyOf(FindExpr, RFindExpr)).bind("find_expr"); + // 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)); + }; + + const auto CompareExpr = cxxMemberCallExpr( + // A method call with three arguments... + argumentCountIs(3), + // ... where the first argument is zero... + hasArgument(0, ZeroLiteral), + // ... named compare... + 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"))); Finder->addMatcher( - // Match [=!]= with a zero on one side and a string.(r?)find on the other. - binaryOperator(hasAnyOperatorName("==", "!="), - hasOperands(FindOrRFindExpr, ZeroLiteral)) + // Match [=!]= with a zero on one side and (r?)find|compare on the other. + binaryOperator( + hasAnyOperatorName("==", "!="), + hasOperands(cxxMemberCallExpr(anyOf(FindExpr, RFindExpr, CompareExpr)) + .bind("find_expr"), + ZeroLiteral)) .bind("expr"), this); } @@ -69,9 +124,28 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) { 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("search_expr"); const auto *StartsWithFunction = Result.Nodes.getNodeAs("starts_with_fun"); + const auto *StringLiteralArg = + Result.Nodes.getNodeAs("string_literal_arg"); + const auto *IntegerLiteralSizeArg = + Result.Nodes.getNodeAs("integer_literal_size_arg"); + const auto *StrlenArg = Result.Nodes.getNodeAs("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; + } + if (ComparisonExpr->getBeginLoc().isMacroID()) { return; } @@ -79,13 +153,13 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) { const bool Neg = ComparisonExpr->getOpcode() == BO_NE; auto Diagnostic = - diag(FindExpr->getBeginLoc(), "use %0 instead of %1() %select{==|!=}2 0") + diag(FindExpr->getExprLoc(), "use %0 instead of %1() %select{==|!=}2 0") << StartsWithFunction->getName() << FindFun->getName() << Neg; - // Remove possible zero second argument and ' [!=]= 0' suffix. + // Remove possible arguments after search expression and ' [!=]= 0' suffix. Diagnostic << FixItHint::CreateReplacement( CharSourceRange::getTokenRange( - Lexer::getLocForEndOfToken(FindExpr->getArg(0)->getEndLoc(), 0, + Lexer::getLocForEndOfToken(SearchExpr->getEndLoc(), 0, *Result.SourceManager, getLangOpts()), ComparisonExpr->getEndLoc()), ")"); @@ -94,11 +168,12 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) { Diagnostic << FixItHint::CreateRemoval(CharSourceRange::getCharRange( ComparisonExpr->getBeginLoc(), FindExpr->getBeginLoc())); - // Replace '(r?)find' with 'starts_with'. + // Replace method name by 'starts_with'. + // Remove possible arguments before search expression. Diagnostic << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(FindExpr->getExprLoc(), - FindExpr->getExprLoc()), - StartsWithFunction->getName()); + CharSourceRange::getCharRange(FindExpr->getExprLoc(), + SearchExpr->getBeginLoc()), + (StartsWithFunction->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 34e9717..840191f 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.h @@ -13,9 +13,10 @@ namespace clang::tidy::modernize { -/// Checks whether a ``find`` or ``rfind`` result is compared with 0 and -/// suggests replacing with ``starts_with`` when the method exists in the class. -/// Notably, this will work with ``std::string`` and ``std::string_view``. +/// 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``. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-starts-ends-with.html diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 28840b9..dbfdb50 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -266,6 +266,10 @@ Changes in existing checks ` check to also remove any trailing whitespace when deleting the ``virtual`` keyword. +- Improved :doc:`modernize-use-starts-ends-with + ` check to also handle + calls to ``compare`` method. + - Improved :doc:`modernize-use-using ` check by adding support for detection of typedefs declared on function level. 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 7f8a262..34237ed 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 @@ -3,15 +3,16 @@ modernize-use-starts-ends-with ============================== -Checks whether a ``find`` or ``rfind`` result is compared with 0 and suggests -replacing with ``starts_with`` when the method exists in the class. Notably, -this will work with ``std::string`` and ``std::string_view``. +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``. .. code-block:: c++ 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 */ } becomes @@ -20,3 +21,4 @@ becomes std::string s = "..."; if (s.starts_with("prefix")) { /* do something */ } if (s.starts_with("prefix")) { /* do something */ } + if (s.starts_with("prefix")) { /* 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 28e2b4a..d031f27 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string @@ -44,6 +44,8 @@ struct basic_string { int compare(const C* s) const; int compare(size_type pos, size_type len, const _Type&) const; int compare(size_type pos, size_type len, const C* s) const; + template + int compare(size_type pos1, size_type count1, const StringViewLike& t) const; size_type find(const _Type& str, size_type pos = 0) const; size_type find(const C* s, size_type pos = 0) const; @@ -129,6 +131,8 @@ bool operator!=(const char*, const std::string&); bool operator==(const std::wstring&, const std::wstring&); bool operator==(const std::wstring&, const wchar_t*); bool operator==(const wchar_t*, const std::wstring&); + +size_t strlen(const char* str); } #endif // _STRING_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h index 4ab7e93..af20586 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h @@ -12,5 +12,6 @@ #include "stddef.h" void *memcpy(void *dest, const void *src, size_t n); +size_t strlen(const char* str); #endif // _STRING_H_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/abseil/redundant-strcat-calls.cpp b/clang-tools-extra/test/clang-tidy/checkers/abseil/redundant-strcat-calls.cpp index ecd17bb..dbd354b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/abseil/redundant-strcat-calls.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/abseil/redundant-strcat-calls.cpp @@ -1,8 +1,6 @@ // RUN: %check_clang_tidy %s abseil-redundant-strcat-calls %t -- -- -isystem %clang_tidy_headers #include -int strlen(const char *); - namespace absl { class string_view { 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 65ed9ed..c5b2c86 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 @@ -1,6 +1,7 @@ // RUN: %check_clang_tidy -std=c++20 %s modernize-use-starts-ends-with %t -- \ // RUN: -- -isystem %clang_tidy_headers +#include #include std::string foo(std::string); @@ -158,10 +159,64 @@ void test(std::string s, std::string_view sv, sub_string ss, sub_sub_string sss, // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use startsWith // CHECK-FIXES: puvi.startsWith("a"); + s.compare(0, 1, "a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with instead of compare() == 0 + // 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-FIXES: !s.starts_with("a"); + + s.compare(0, strlen("a"), "a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with("a"); + + s.compare(0, std::strlen("a"), "a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with("a"); + + s.compare(0, std::strlen(("a")), "a") == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with("a"); + + s.compare(0, std::strlen(("a")), (("a"))) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with("a"); + + s.compare(0, s.size(), s) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(s); + + s.compare(0, s.length(), s) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(s); + + 0 != s.compare(0, sv.length(), sv); + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(sv); + + #define LENGTH(x) (x).length() + s.compare(0, LENGTH(s), s) == 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(s); + + s.compare(ZERO, LENGTH(s), s) == ZERO; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: s.starts_with(s); + + s.compare(ZERO, LENGTH(sv), sv) != 0; + // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: use starts_with + // CHECK-FIXES: !s.starts_with(sv); + // Expressions that don't trigger the check are here. #define EQ(x, y) ((x) == (y)) EQ(s.find("a"), 0); #define DOTFIND(x, y) (x).find(y) DOTFIND(s, "a") == 0; + + #define STARTS_WITH_COMPARE(x, y) (x).compare(0, (x).size(), (y)) + STARTS_WITH_COMPARE(s, s) == 0; + + s.compare(0, 1, "ab") == 0; } -- cgit v1.1