[clang-tidy] Enhance modernize-use-starts-ends-with to handle substr patterns

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<BinaryOperator>(ComparisonExpr))
+    return BO->getOpcode() == BO_NE;
+
+  if (const auto *Op = llvm::dyn_cast<CXXOperatorCallExpr>(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<BinaryOperator>("expr");
+  const auto *ComparisonExpr = Result.Nodes.getNodeAs<Expr>("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>("needle");
@@ -183,39 +210,42 @@ void UseStartsEndsWithCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *EndsWithFunction =
       Result.Nodes.getNodeAs<CXXMethodDecl>("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(), "!");
 }
 

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
-  <clang-tidy/checks/modernize/use-starts-ends-with>` check to handle two cases
-  that can be replaced with ``ends_with``
+  <clang-tidy/checks/modernize/use-starts-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
   <clang-tidy/checks/modernize/use-std-format>` check to support replacing

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)``
+==================================================== =====================

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<C, T> sv) const noexcept;
   constexpr bool starts_with(C ch) const noexcept;
   constexpr bool starts_with(const C* s) const;

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";
 }