[clang-tidy][modernize-use-starts-ends-with] Add support for compare()

nicovank · nicovank · commit ff7ebb3086d5 · 2024-04-21T21:12:11.000Z
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp
@@ -16,6 +16,75 @@
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::modernize {
+namespace {
+// Given two argument indices X and Y, matches when a call expression has a
+// string at index X with an expression representing that string's length at
+// index Y. The string can be a string literal or a variable. The length can be
+// matched via an integer literal or a call to strlen() in the case of a string
+// literal, and by a call to size() or length() in the string variable case.
+AST_POLYMORPHIC_MATCHER_P2(hasStringAndLengthArguments,
+                           AST_POLYMORPHIC_SUPPORTED_TYPES(
+                               CallExpr, CXXConstructExpr,
+                               CXXUnresolvedConstructExpr, ObjCMessageExpr),
+                           unsigned, StringArgIndex, unsigned, LengthArgIndex) {
+  if (StringArgIndex >= Node.getNumArgs() ||
+      LengthArgIndex >= Node.getNumArgs()) {
+    return false;
+  }
+
+  const Expr *StringArgExpr =
+      Node.getArg(StringArgIndex)->IgnoreParenImpCasts();
+  const Expr *LengthArgExpr =
+      Node.getArg(LengthArgIndex)->IgnoreParenImpCasts();
+
+  if (const auto *StringArg = dyn_cast<StringLiteral>(StringArgExpr)) {
+    // Match an integer literal equal to the string length or a call to strlen.
+
+    static const auto Matcher = expr(anyOf(
+        integerLiteral().bind("integer_literal_size"),
+        callExpr(callee(functionDecl(hasName("strlen"))), argumentCountIs(1),
+                 hasArgument(0, stringLiteral().bind("strlen_arg")))));
+
+    if (!Matcher.matches(*LengthArgExpr, Finder, Builder)) {
+      return false;
+    }
+
+    return Builder->removeBindings(
+        [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+          const auto *IntegerLiteralSize =
+              Nodes.getNodeAs<IntegerLiteral>("integer_literal_size");
+          const auto *StrlenArg = Nodes.getNodeAs<StringLiteral>("strlen_arg");
+          if (IntegerLiteralSize) {
+            return IntegerLiteralSize->getValue().getZExtValue() !=
+                   StringArg->getLength();
+          }
+          return StrlenArg->getLength() != StringArg->getLength();
+        });
+  }
+
+  if (const auto *StringArg = dyn_cast<DeclRefExpr>(StringArgExpr)) {
+    // Match a call to size() or length() on the same variable.
+
+    static const auto Matcher = cxxMemberCallExpr(
+        on(declRefExpr(to(varDecl().bind("string_var_decl")))),
+        callee(cxxMethodDecl(hasAnyName("size", "length"), isConst(),
+                             parameterCountIs(0))));
+
+    if (!Matcher.matches(*LengthArgExpr, Finder, Builder)) {
+      return false;
+    }
+
+    return Builder->removeBindings(
+        [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+          const auto *StringVarDecl =
+              Nodes.getNodeAs<VarDecl>("string_var_decl");
+          return StringVarDecl != StringArg->getDecl();
+        });
+  }
+
+  return false;
+}
+} // namespace
 
 UseStartsEndsWithCheck::UseStartsEndsWithCheck(StringRef Name,
                                                ClangTidyContext *Context)
@@ -43,7 +112,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 +123,30 @@ 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)))));
-
-  const auto FindOrRFindExpr =
-      cxxMemberCallExpr(anyOf(FindExpr, RFindExpr)).bind("find_expr");
+          hasCanonicalType(hasDeclaration(ClassWithStartsWithFunction)))),
+      // Bind search expression.
+      hasArgument(0, expr().bind("search_expr")));
+
+  const auto CompareExpr = cxxMemberCallExpr(
+      // A method call with a first argument of 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 its length.
+      hasStringAndLengthArguments(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,6 +155,7 @@ 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 *StartsWithFunction =
       Result.Nodes.getNodeAs<CXXMethodDecl>("starts_with_fun");
 
@@ -79,13 +166,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 +181,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
@@ -13,8 +13,8 @@
 
 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.
+/// 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:
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -261,6 +261,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-override>` check to also remove any trailing
   whitespace when deleting the ``virtual`` keyword.
 
+- Improved :doc:`modernize-use-starts-ends-with
+  <clang-tidy/checks/modernize/use-starts-ends-with>` check to also handle
+  calls to ``compare`` method.
+
 - Improved :doc:`modernize-use-using <clang-tidy/checks/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
@@ -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,
+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
@@ -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<class StringViewLike>
+  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
@@ -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
@@ -1,8 +1,6 @@
 // RUN: %check_clang_tidy %s abseil-redundant-strcat-calls %t -- -- -isystem %clang_tidy_headers
 #include <string>
 
-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
@@ -1,6 +1,7 @@
 // RUN: %check_clang_tidy -std=c++20 %s modernize-use-starts-ends-with %t -- \
 // RUN:   -- -isystem %clang_tidy_headers
 
+#include <string.h>
 #include <string>
 
 std::string foo(std::string);
@@ -158,10 +159,54 @@ 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, 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;
 }
