[clang-tidy] check `std::string_view` and custom string-like classes in `readability-string-compare` #88636

vvd170501 · 2024-04-13T21:08:26Z

This PR aims to expand the list of classes that are considered to be "strings" by readability-string-compare check.

Currently only std::string;:compare is checked, but std::string_view has a similar compare method. This PR enables checking of std::string_view::compare by default.
Some codebases use custom string-like classes that have public interfaces similar to std::string or std::string_view. Example: TStringBase, A new option, readability-string-compare.StringClassNames, is added to allow specifying a custom list of string-like classes.

Related to, but does not solve #28396 (only adds support for custom string-like classes, not custom functions)

clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp

llvmbot · 2024-04-13T22:21:35Z

@llvm/pr-subscribers-clang-tidy

Author: Vadim D. (vvd170501)

Changes

This PR aims to expand the list of classes that are considered to be "strings" by readability-string-compare check.

Currently only std::string;:compare is checked, but std::string_view has a similar compare method. This PR enables checking of std::string_view::compare by default.
Some codebases use custom string-like classes that have public interfaces similar to std::string or std::string_view. Example: TStringBase, A new option, readability-string-compare.StringClassNames, is added to allow specifying a custom list of string-like classes.

Related to, but does not solve #28396 (only adds support for custom string-like classes, not custom functions)

Full diff: https://github.com/llvm/llvm-project/pull/88636.diff

7 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp (+51-23)
(modified) clang-tools-extra/clang-tidy/readability/StringCompareCheck.h (+7-2)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
(modified) clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst (+31-2)
(modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string (+10)
(added) clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp (+38)
(modified) clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp (+23)

diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
index 3b5d89c8c64719..905e5b156ef864 100644
--- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
@@ -7,12 +7,16 @@
 //===----------------------------------------------------------------------===//
 
 #include "StringCompareCheck.h"
-#include "../utils/FixItHintUtils.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/FixIt.h"
+#include "llvm/ADT/StringRef.h"
+#include <vector>
 
 using namespace clang::ast_matchers;
+namespace optutils = clang::tidy::utils::options;
 
 namespace clang::tidy::readability {
 
@@ -20,29 +24,53 @@ static const StringRef CompareMessage = "do not use 'compare' to test equality "
                                         "of strings; use the string equality "
                                         "operator instead";
 
+static const std::vector<StringRef> StringClasses = {
+    "::std::basic_string", "::std::basic_string_view"};
+
+StringCompareCheck::StringCompareCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StringLikeClasses(
+          optutils::parseStringList(Options.get("StringLikeClasses", ""))) {}
+
 void StringCompareCheck::registerMatchers(MatchFinder *Finder) {
-  const auto StrCompare = cxxMemberCallExpr(
-      callee(cxxMethodDecl(hasName("compare"),
-                           ofClass(classTemplateSpecializationDecl(
-                               hasName("::std::basic_string"))))),
-      hasArgument(0, expr().bind("str2")), argumentCountIs(1),
-      callee(memberExpr().bind("str1")));
-
-  // First and second case: cast str.compare(str) to boolean.
-  Finder->addMatcher(
-      traverse(TK_AsIs,
-               implicitCastExpr(hasImplicitDestinationType(booleanType()),
-                                has(StrCompare))
-                   .bind("match1")),
-      this);
-
-  // Third and fourth case: str.compare(str) == 0 and str.compare(str) != 0.
-  Finder->addMatcher(
-      binaryOperator(hasAnyOperatorName("==", "!="),
-                     hasOperands(StrCompare.bind("compare"),
-                                 integerLiteral(equals(0)).bind("zero")))
-          .bind("match2"),
-      this);
+  const auto RegisterForClasses = [&, this](const auto &StringClassMatcher) {
+    const auto StrCompare = cxxMemberCallExpr(
+        callee(cxxMethodDecl(hasName("compare"), ofClass(StringClassMatcher))),
+        hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+        callee(memberExpr().bind("str1")));
+
+    // First and second case: cast str.compare(str) to boolean.
+    Finder->addMatcher(
+        traverse(TK_AsIs,
+                 implicitCastExpr(hasImplicitDestinationType(booleanType()),
+                                  has(StrCompare))
+                     .bind("match1")),
+        this);
+
+    // Third and fourth case: str.compare(str) == 0
+    // and str.compare(str) !=  0.
+    Finder->addMatcher(
+        binaryOperator(hasAnyOperatorName("==", "!="),
+                       hasOperands(StrCompare.bind("compare"),
+                                   integerLiteral(equals(0)).bind("zero")))
+            .bind("match2"),
+        this);
+  };
+  if (StringLikeClasses.empty()) {
+    RegisterForClasses(
+        classTemplateSpecializationDecl(hasAnyName(StringClasses)));
+  } else {
+    // StringLikeClasses may or may not be templates, so we need to match both
+    // template and non-template classes.
+    std::vector<StringRef> PossiblyTemplateClasses = StringClasses;
+    PossiblyTemplateClasses.insert(PossiblyTemplateClasses.end(),
+                                   StringLikeClasses.begin(),
+                                   StringLikeClasses.end());
+    RegisterForClasses(anyOf(
+        classTemplateSpecializationDecl(hasAnyName(PossiblyTemplateClasses)),
+        cxxRecordDecl(hasAnyName(StringLikeClasses))));
+  }
 }
 
 void StringCompareCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h
index 812736d806b71d..b7bb5a0097645b 100644
--- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRINGCOMPARECHECK_H
 
 #include "../ClangTidyCheck.h"
+#include <vector>
 
 namespace clang::tidy::readability {
 
@@ -20,13 +21,17 @@ namespace clang::tidy::readability {
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability/string-compare.html
 class StringCompareCheck : public ClangTidyCheck {
 public:
-  StringCompareCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  StringCompareCheck(StringRef Name, ClangTidyContext *Context);
+
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
+
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::vector<StringRef> StringLikeClasses;
 };
 
 } // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 1405fb0df1f8dd..cbafff01b58592 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -286,6 +286,11 @@ Changes in existing checks
   check by resolving fix-it overlaps in template code by disregarding implicit
   instances.
 
+- Improved :doc:`readability-string-compare
+  <clang-tidy/checks/readability/string-compare>` check to also detect
+  usages of `std::string_view::compare`. Added a `StringLikeClasses` option
+  to detect usages of `compare` method in custom string-like classes.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst
index 268632eee61a27..f86ea6704ad56e 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst
@@ -14,10 +14,12 @@ recommended to avoid the risk of incorrect interpretation of the return value
 and to simplify the code. The string equality and inequality operators can
 also be faster than the ``compare`` method due to early termination.
 
-Examples:
+Example
+-------
 
 .. code-block:: c++
 
+  // The same rules apply to std::string_view.
   std::string str1{"a"};
   std::string str2{"b"};
 
@@ -50,5 +52,32 @@ Examples:
   }
 
 The above code examples show the list of if-statements that this check will
-give a warning for. All of them uses ``compare`` to check if equality or
+give a warning for. All of them use ``compare`` to check equality or
 inequality of two strings instead of using the correct operators.
+
+Options
+-------
+
+.. option:: StringLikeClasses
+
+   A string containing comma-separated names of string-like classes. Default is an empty string.
+   If a class from this list has a ``compare`` method similar to ``std::string``, it will be checked in the same way.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+  struct CustomString {
+  public:
+    int compare (const CustomString& other) const;
+  }
+
+  CustomString str1;
+  CustomString str2;
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+If `StringLikeClasses` contains ``CustomString``, the check will suggest replacing ``compare`` with equality operator.
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 28e2b4a231e52e..bc58fdad3fd44e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -106,6 +106,8 @@ struct basic_string_view {
   constexpr bool starts_with(C ch) const noexcept;
   constexpr bool starts_with(const C* s) const;
 
+  constexpr int compare(basic_string_view sv) const noexcept;
+
   static constexpr size_t npos = -1;
 };
 
@@ -129,6 +131,14 @@ 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&);
+
+bool operator==(const std::string_view&, const std::string_view&);
+bool operator==(const std::string_view&, const char*);
+bool operator==(const char*, const std::string_view&);
+
+bool operator!=(const std::string_view&, const std::string_view&);
+bool operator!=(const std::string_view&, const char*);
+bool operator!=(const char*, const std::string_view&);
 }
 
 #endif // _STRING_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp
new file mode 100644
index 00000000000000..2bb47d33da389d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy %s readability-string-compare %t -- -config='{CheckOptions: {readability-string-compare.StringLikeClasses: "CustomStringTemplateBase;CustomStringNonTemplateBase"}}' -- -isystem %clang_tidy_headers
+#include <string>
+
+struct CustomStringNonTemplateBase {
+  int compare(const CustomStringNonTemplateBase& Other) const {
+    return 123;  // value is not important for check
+  }
+};
+
+template <typename T>
+struct CustomStringTemplateBase {
+  int compare(const CustomStringTemplateBase& Other) const {
+    return 123;
+  }
+};
+
+struct CustomString1 : CustomStringNonTemplateBase {};
+struct CustomString2 : CustomStringTemplateBase<char> {};
+
+void StdClassesAreStillDetected() {
+  std::string_view sv1("a");
+  std::string_view sv2("b");
+  if (sv1.compare(sv2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+}
+
+void CustomStringClasses() {
+  CustomString1 custom1;
+  if (custom1.compare(custom1)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+
+  CustomString2 custom2;
+  if (custom2.compare(custom2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp
index 2c08b86cf72fa0..c4fea4341617b8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp
@@ -67,11 +67,27 @@ void Test() {
   if (str1.compare(comp())) {
   }
   // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+
+  std::string_view sv1("a");
+  std::string_view sv2("b");
+  if (sv1.compare(sv2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+}
+
+struct DerivedFromStdString : std::string {};
+
+void TestDerivedClass() {
+  DerivedFromStdString derived;
+  if (derived.compare(derived)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
 }
 
 void Valid() {
   std::string str1("a", 1);
   std::string str2("b", 1);
+
   if (str1 == str2) {
   }
   if (str1 != str2) {
@@ -96,4 +112,11 @@ void Valid() {
   }
   if (str1.compare(str2) == -1) {
   }
+
+  std::string_view sv1("a");
+  std::string_view sv2("b");
+  if (sv1 == sv2) {
+  }
+  if (sv1.compare(sv2) > 0) {
+  }
 }

llvmbot · 2024-04-13T22:21:35Z

@llvm/pr-subscribers-clang-tools-extra

Author: Vadim D. (vvd170501)

Changes

This PR aims to expand the list of classes that are considered to be "strings" by readability-string-compare check.

Currently only std::string;:compare is checked, but std::string_view has a similar compare method. This PR enables checking of std::string_view::compare by default.
Some codebases use custom string-like classes that have public interfaces similar to std::string or std::string_view. Example: TStringBase, A new option, readability-string-compare.StringClassNames, is added to allow specifying a custom list of string-like classes.

Related to, but does not solve #28396 (only adds support for custom string-like classes, not custom functions)

Full diff: https://github.com/llvm/llvm-project/pull/88636.diff

7 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp (+51-23)
(modified) clang-tools-extra/clang-tidy/readability/StringCompareCheck.h (+7-2)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
(modified) clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst (+31-2)
(modified) clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string (+10)
(added) clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp (+38)
(modified) clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp (+23)

diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
index 3b5d89c8c64719..905e5b156ef864 100644
--- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
@@ -7,12 +7,16 @@
 //===----------------------------------------------------------------------===//
 
 #include "StringCompareCheck.h"
-#include "../utils/FixItHintUtils.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/FixIt.h"
+#include "llvm/ADT/StringRef.h"
+#include <vector>
 
 using namespace clang::ast_matchers;
+namespace optutils = clang::tidy::utils::options;
 
 namespace clang::tidy::readability {
 
@@ -20,29 +24,53 @@ static const StringRef CompareMessage = "do not use 'compare' to test equality "
                                         "of strings; use the string equality "
                                         "operator instead";
 
+static const std::vector<StringRef> StringClasses = {
+    "::std::basic_string", "::std::basic_string_view"};
+
+StringCompareCheck::StringCompareCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StringLikeClasses(
+          optutils::parseStringList(Options.get("StringLikeClasses", ""))) {}
+
 void StringCompareCheck::registerMatchers(MatchFinder *Finder) {
-  const auto StrCompare = cxxMemberCallExpr(
-      callee(cxxMethodDecl(hasName("compare"),
-                           ofClass(classTemplateSpecializationDecl(
-                               hasName("::std::basic_string"))))),
-      hasArgument(0, expr().bind("str2")), argumentCountIs(1),
-      callee(memberExpr().bind("str1")));
-
-  // First and second case: cast str.compare(str) to boolean.
-  Finder->addMatcher(
-      traverse(TK_AsIs,
-               implicitCastExpr(hasImplicitDestinationType(booleanType()),
-                                has(StrCompare))
-                   .bind("match1")),
-      this);
-
-  // Third and fourth case: str.compare(str) == 0 and str.compare(str) != 0.
-  Finder->addMatcher(
-      binaryOperator(hasAnyOperatorName("==", "!="),
-                     hasOperands(StrCompare.bind("compare"),
-                                 integerLiteral(equals(0)).bind("zero")))
-          .bind("match2"),
-      this);
+  const auto RegisterForClasses = [&, this](const auto &StringClassMatcher) {
+    const auto StrCompare = cxxMemberCallExpr(
+        callee(cxxMethodDecl(hasName("compare"), ofClass(StringClassMatcher))),
+        hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+        callee(memberExpr().bind("str1")));
+
+    // First and second case: cast str.compare(str) to boolean.
+    Finder->addMatcher(
+        traverse(TK_AsIs,
+                 implicitCastExpr(hasImplicitDestinationType(booleanType()),
+                                  has(StrCompare))
+                     .bind("match1")),
+        this);
+
+    // Third and fourth case: str.compare(str) == 0
+    // and str.compare(str) !=  0.
+    Finder->addMatcher(
+        binaryOperator(hasAnyOperatorName("==", "!="),
+                       hasOperands(StrCompare.bind("compare"),
+                                   integerLiteral(equals(0)).bind("zero")))
+            .bind("match2"),
+        this);
+  };
+  if (StringLikeClasses.empty()) {
+    RegisterForClasses(
+        classTemplateSpecializationDecl(hasAnyName(StringClasses)));
+  } else {
+    // StringLikeClasses may or may not be templates, so we need to match both
+    // template and non-template classes.
+    std::vector<StringRef> PossiblyTemplateClasses = StringClasses;
+    PossiblyTemplateClasses.insert(PossiblyTemplateClasses.end(),
+                                   StringLikeClasses.begin(),
+                                   StringLikeClasses.end());
+    RegisterForClasses(anyOf(
+        classTemplateSpecializationDecl(hasAnyName(PossiblyTemplateClasses)),
+        cxxRecordDecl(hasAnyName(StringLikeClasses))));
+  }
 }
 
 void StringCompareCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h
index 812736d806b71d..b7bb5a0097645b 100644
--- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRINGCOMPARECHECK_H
 
 #include "../ClangTidyCheck.h"
+#include <vector>
 
 namespace clang::tidy::readability {
 
@@ -20,13 +21,17 @@ namespace clang::tidy::readability {
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability/string-compare.html
 class StringCompareCheck : public ClangTidyCheck {
 public:
-  StringCompareCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  StringCompareCheck(StringRef Name, ClangTidyContext *Context);
+
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
+
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::vector<StringRef> StringLikeClasses;
 };
 
 } // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 1405fb0df1f8dd..cbafff01b58592 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -286,6 +286,11 @@ Changes in existing checks
   check by resolving fix-it overlaps in template code by disregarding implicit
   instances.
 
+- Improved :doc:`readability-string-compare
+  <clang-tidy/checks/readability/string-compare>` check to also detect
+  usages of `std::string_view::compare`. Added a `StringLikeClasses` option
+  to detect usages of `compare` method in custom string-like classes.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst
index 268632eee61a27..f86ea6704ad56e 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst
@@ -14,10 +14,12 @@ recommended to avoid the risk of incorrect interpretation of the return value
 and to simplify the code. The string equality and inequality operators can
 also be faster than the ``compare`` method due to early termination.
 
-Examples:
+Example
+-------
 
 .. code-block:: c++
 
+  // The same rules apply to std::string_view.
   std::string str1{"a"};
   std::string str2{"b"};
 
@@ -50,5 +52,32 @@ Examples:
   }
 
 The above code examples show the list of if-statements that this check will
-give a warning for. All of them uses ``compare`` to check if equality or
+give a warning for. All of them use ``compare`` to check equality or
 inequality of two strings instead of using the correct operators.
+
+Options
+-------
+
+.. option:: StringLikeClasses
+
+   A string containing comma-separated names of string-like classes. Default is an empty string.
+   If a class from this list has a ``compare`` method similar to ``std::string``, it will be checked in the same way.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+  struct CustomString {
+  public:
+    int compare (const CustomString& other) const;
+  }
+
+  CustomString str1;
+  CustomString str2;
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+If `StringLikeClasses` contains ``CustomString``, the check will suggest replacing ``compare`` with equality operator.
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 28e2b4a231e52e..bc58fdad3fd44e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
@@ -106,6 +106,8 @@ struct basic_string_view {
   constexpr bool starts_with(C ch) const noexcept;
   constexpr bool starts_with(const C* s) const;
 
+  constexpr int compare(basic_string_view sv) const noexcept;
+
   static constexpr size_t npos = -1;
 };
 
@@ -129,6 +131,14 @@ 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&);
+
+bool operator==(const std::string_view&, const std::string_view&);
+bool operator==(const std::string_view&, const char*);
+bool operator==(const char*, const std::string_view&);
+
+bool operator!=(const std::string_view&, const std::string_view&);
+bool operator!=(const std::string_view&, const char*);
+bool operator!=(const char*, const std::string_view&);
 }
 
 #endif // _STRING_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp
new file mode 100644
index 00000000000000..2bb47d33da389d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy %s readability-string-compare %t -- -config='{CheckOptions: {readability-string-compare.StringLikeClasses: "CustomStringTemplateBase;CustomStringNonTemplateBase"}}' -- -isystem %clang_tidy_headers
+#include <string>
+
+struct CustomStringNonTemplateBase {
+  int compare(const CustomStringNonTemplateBase& Other) const {
+    return 123;  // value is not important for check
+  }
+};
+
+template <typename T>
+struct CustomStringTemplateBase {
+  int compare(const CustomStringTemplateBase& Other) const {
+    return 123;
+  }
+};
+
+struct CustomString1 : CustomStringNonTemplateBase {};
+struct CustomString2 : CustomStringTemplateBase<char> {};
+
+void StdClassesAreStillDetected() {
+  std::string_view sv1("a");
+  std::string_view sv2("b");
+  if (sv1.compare(sv2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+}
+
+void CustomStringClasses() {
+  CustomString1 custom1;
+  if (custom1.compare(custom1)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+
+  CustomString2 custom2;
+  if (custom2.compare(custom2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp
index 2c08b86cf72fa0..c4fea4341617b8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare.cpp
@@ -67,11 +67,27 @@ void Test() {
   if (str1.compare(comp())) {
   }
   // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+
+  std::string_view sv1("a");
+  std::string_view sv2("b");
+  if (sv1.compare(sv2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
+}
+
+struct DerivedFromStdString : std::string {};
+
+void TestDerivedClass() {
+  DerivedFromStdString derived;
+  if (derived.compare(derived)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [readability-string-compare]
 }
 
 void Valid() {
   std::string str1("a", 1);
   std::string str2("b", 1);
+
   if (str1 == str2) {
   }
   if (str1 != str2) {
@@ -96,4 +112,11 @@ void Valid() {
   }
   if (str1.compare(str2) == -1) {
   }
+
+  std::string_view sv1("a");
+  std::string_view sv2("b");
+  if (sv1 == sv2) {
+  }
+  if (sv1.compare(sv2) > 0) {
+  }
 }

clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst

PiotrZSL

Missing storeOptions
Some other tiny nits.
Would be nice to get rid of lambda RegisterForClasses as it's not needed, and have common handling.

clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp

clang-tools-extra/clang-tidy/readability/StringCompareCheck.h

vvd170501 · 2024-04-22T08:48:52Z

Ping.
@PiotrZSL, I've added the changes you requested (except matchesAnyListedName), waiting for your feedback

PiotrZSL

Looks fine for me.

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst

…ng-like classes Co-authored-by: EugeneZelenko <[email protected]>

vvd170501 · 2024-04-26T21:23:30Z

@PiotrZSL, can you merge this, please?
I've fixed merge conflicts caused by ef59069 (changes in the string header for tests), now everything should be ok

vvd170501 · 2024-05-09T08:22:02Z

@EugeneZelenko, can you approve this, please? Or are 2 approves enough to merge?

vvd170501 force-pushed the readability-string-compare-additional-classes branch 3 times, most recently from b63dd11 to 0db24a6 Compare April 13, 2024 22:14

vvd170501 commented Apr 13, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp Outdated Show resolved Hide resolved

vvd170501 marked this pull request as ready for review April 13, 2024 22:21

llvmbot added clang-tools-extra clang-tidy labels Apr 13, 2024

EugeneZelenko reviewed Apr 14, 2024

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst Outdated Show resolved Hide resolved

clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst Outdated Show resolved Hide resolved

EugeneZelenko requested review from PiotrZSL and HerrCai0907 April 14, 2024 14:17

PiotrZSL requested changes Apr 14, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/StringCompareCheck.h Show resolved Hide resolved

vvd170501 requested a review from PiotrZSL April 14, 2024 21:44

PiotrZSL approved these changes Apr 22, 2024

View reviewed changes

EugeneZelenko reviewed Apr 22, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

EugeneZelenko reviewed Apr 22, 2024

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/readability/string-compare.rst Show resolved Hide resolved

vvd170501 force-pushed the readability-string-compare-additional-classes branch from a0bab74 to ab87ad1 Compare April 24, 2024 10:02

readability-string-compare: check std::string_view, allow custom stri…

54b68bd

…ng-like classes Co-authored-by: EugeneZelenko <[email protected]>

vvd170501 force-pushed the readability-string-compare-additional-classes branch from ab87ad1 to 54b68bd Compare April 24, 2024 19:36

HerrCai0907 approved these changes Apr 27, 2024

View reviewed changes

PiotrZSL merged commit febd89c into llvm:main May 9, 2024

vvd170501 deleted the readability-string-compare-additional-classes branch May 10, 2024 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] check `std::string_view` and custom string-like classes in `readability-string-compare` #88636

[clang-tidy] check `std::string_view` and custom string-like classes in `readability-string-compare` #88636

Uh oh!

vvd170501 commented Apr 13, 2024 •

edited

Loading

Uh oh!

Uh oh!

llvmbot commented Apr 13, 2024

Uh oh!

llvmbot commented Apr 13, 2024

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvd170501 commented Apr 22, 2024

Uh oh!

PiotrZSL left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvd170501 commented Apr 26, 2024

Uh oh!

vvd170501 commented May 9, 2024

Uh oh!

Uh oh!

[clang-tidy] check std::string_view and custom string-like classes in readability-string-compare #88636

[clang-tidy] check std::string_view and custom string-like classes in readability-string-compare #88636

Uh oh!

Conversation

vvd170501 commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Apr 13, 2024

Uh oh!

llvmbot commented Apr 13, 2024

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvd170501 commented Apr 22, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvd170501 commented Apr 26, 2024

Uh oh!

vvd170501 commented May 9, 2024

Uh oh!

Uh oh!

[clang-tidy] check `std::string_view` and custom string-like classes in `readability-string-compare` #88636

[clang-tidy] check `std::string_view` and custom string-like classes in `readability-string-compare` #88636

vvd170501 commented Apr 13, 2024 •

edited

Loading