[clang-tools-extra] [clang-tidy] check `std::string_view` and custom string-like classes in `readability-string-compare` (PR #88636)
Vadim D. via cfe-commits
cfe-commits at lists.llvm.org
Sat Apr 13 14:08:33 PDT 2024
https://github.com/vvd170501 created https://github.com/llvm/llvm-project/pull/88636
This PR aims to expand the list of classes that are considered to be "strings" by `readability-string-compare` check.
1. Currently `readability-string-compare` only checks `std::string;:compare`, but `std::string_view` has a similar `compare` method, which also should not be used to check equality of two strings.
2. Some codebases use custom string-like classes that have public interfaces similar to `std::string` or `std::string_view`. Example: [TStringBase](https://github.com/yandex/yatool/blob/main/util/generic/strbase.h#L38),
Related to, but does not solve #28396 (only adds support for custom string-like classes, not custom functions)
>From eb5279fd83ba114b8eb094099d5993d6bfb003e3 Mon Sep 17 00:00:00 2001
From: Vadim Dudkin <vvd170501 at gmail.com>
Date: Sat, 13 Apr 2024 23:36:12 +0300
Subject: [PATCH] readability-string-compare: check std::string_view, allow
custom string-like classes
---
.../readability/StringCompareCheck.cpp | 75 +++++++++++++------
.../readability/StringCompareCheck.h | 10 ++-
.../clang-tidy/checkers/Inputs/Headers/string | 10 +++
.../string-compare-custom-string-classes.cpp | 41 ++++++++++
.../checkers/readability/string-compare.cpp | 14 ++++
5 files changed, 125 insertions(+), 25 deletions(-)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
index 3b5d89c8c64719..30bde1f8b75b61 100644
--- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp
@@ -7,12 +7,15 @@
//===----------------------------------------------------------------------===//
#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"
using namespace clang::ast_matchers;
+namespace optutils = clang::tidy::utils::options;
namespace clang::tidy::readability {
@@ -20,29 +23,55 @@ static const StringRef CompareMessage = "do not use 'compare' to test equality "
"of strings; use the string equality "
"operator instead";
+static const StringRef DefaultStringClassNames = "::std::basic_string;"
+ "::std::basic_string_view";
+
+StringCompareCheck::StringCompareCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ StringClassNames(optutils::parseStringList(
+ Options.get("StringClassNames", DefaultStringClassNames))),
+ // Both std::string and std::string_view are templates, so this check only
+ // needs to match template classes by default.
+ // Custom `StringClassNames` may contain non-template classes, and
+ // it's impossible to tell them apart from templates just by name.
+ CheckNonTemplateClasses(Options.get("StringClassNames").has_value()) {}
+
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);
+ if (StringClassNames.empty()) {
+ return;
+ }
+ 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);
+ };
+ auto TemplateClassMatcher =
+ classTemplateSpecializationDecl(hasAnyName(StringClassNames));
+ if (CheckNonTemplateClasses) {
+ RegisterForClasses(anyOf(std::move(TemplateClassMatcher),
+ cxxRecordDecl(hasAnyName(StringClassNames))));
+ } else {
+ RegisterForClasses(std::move(TemplateClassMatcher));
+ }
}
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..5aaaef064948c8 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,18 @@ 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> StringClassNames;
+ const bool CheckNonTemplateClasses;
};
} // namespace clang::tidy::readability
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..20609a350bd1c8
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/string-compare-custom-string-classes.cpp
@@ -0,0 +1,41 @@
+// RUN: %check_clang_tidy %s readability-string-compare %t -- -config='{CheckOptions: {readability-string-compare.StringClassNames: "::std::basic_string;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 DerivedFromStdString : std::string {};
+struct CustomString1 : CustomStringNonTemplateBase {};
+struct CustomString2 : CustomStringTemplateBase<char> {};
+
+void CustomStringClasses() {
+ std::string_view sv1("a");
+ std::string_view sv2("b");
+ if (sv1.compare(sv2)) { // No warning - StringClassNames does not contain string_view.
+ }
+
+ 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]
+
+ 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..52a65aa5415c1d 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,18 @@ 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]
}
void Valid() {
std::string str1("a", 1);
std::string str2("b", 1);
+
if (str1 == str2) {
}
if (str1 != str2) {
@@ -96,4 +103,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) {
+ }
}
More information about the cfe-commits
mailing list