https://github.com/vvd170501 updated https://github.com/llvm/llvm-project/pull/88636
>From 0db24a6806e9429b5e7b9cd9d0777315b3e6d87e Mon Sep 17 00:00:00 2001 From: Vadim Dudkin <vvd170...@gmail.com> Date: Sat, 13 Apr 2024 23:36:12 +0300 Subject: [PATCH 1/5] readability-string-compare: check std::string_view, allow custom string-like classes --- .../readability/StringCompareCheck.cpp | 74 +++++++++++++------ .../readability/StringCompareCheck.h | 9 ++- clang-tools-extra/docs/ReleaseNotes.rst | 5 ++ .../checks/readability/string-compare.rst | 33 ++++++++- .../clang-tidy/checkers/Inputs/Headers/string | 10 +++ .../string-compare-custom-string-classes.cpp | 38 ++++++++++ .../checkers/readability/string-compare.cpp | 23 ++++++ 7 files changed, 165 insertions(+), 27 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..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) { + } } >From a5ba6c0e3e9aac2da77970dadf5292792b4ebb0b Mon Sep 17 00:00:00 2001 From: Vadim Dudkin <vvd170...@gmail.com> Date: Sun, 14 Apr 2024 17:37:51 +0300 Subject: [PATCH 2/5] Fix doc for new option --- .../clang-tidy/checks/readability/string-compare.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 f86ea6704ad56e..a2ff4591693483 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 @@ -60,8 +60,10 @@ 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. + A string containing comma-separated names of string-like classes. + Default value 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 ^^^^^^^ @@ -80,4 +82,5 @@ Example if (str1.compare(str2)) { } -If `StringLikeClasses` contains ``CustomString``, the check will suggest replacing ``compare`` with equality operator. +If `StringLikeClasses` contains ``CustomString``, the check will suggest +replacing ``compare`` with equality operator. >From c716d231d7b8bb23c8a42e7bf0a556a76ac52c04 Mon Sep 17 00:00:00 2001 From: Vadim Dudkin <vvd170...@gmail.com> Date: Mon, 15 Apr 2024 00:28:05 +0300 Subject: [PATCH 3/5] Make StringLikeClasses consistent with existing checks --- .../readability/StringCompareCheck.cpp | 65 ++++++++----------- .../checks/readability/string-compare.rst | 7 +- .../string-compare-custom-string-classes.cpp | 7 +- 3 files changed, 32 insertions(+), 47 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp index 905e5b156ef864..0bcf31a4795d2b 100644 --- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp @@ -24,53 +24,40 @@ 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"}; +static const StringRef DefaultStringLikeClasses = "::std::basic_string;" + "::std::basic_string_view"; StringCompareCheck::StringCompareCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), - StringLikeClasses( - optutils::parseStringList(Options.get("StringLikeClasses", ""))) {} + StringLikeClasses(optutils::parseStringList( + Options.get("StringLikeClasses", DefaultStringLikeClasses))) {} void StringCompareCheck::registerMatchers(MatchFinder *Finder) { - 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)))); + return; } + const auto StrCompare = cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("compare"), ofClass(cxxRecordDecl(hasAnyName( + StringLikeClasses))))), + 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); } void StringCompareCheck::check(const MatchFinder::MatchResult &Result) { 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 a2ff4591693483..4be2473bed2d74 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 @@ -60,9 +60,10 @@ Options .. option:: StringLikeClasses - A string containing comma-separated names of string-like classes. - Default value is an empty string. If a class from this list has a - ``compare`` method similar to ``std::string``, it will be checked + A string containing semicolon-separated names of string-like classes. + By default contains only ``::std::basic_string`` + and ``::std::basic_string_view``. If a class from this list has + a ``compare`` method similar to that of ``std::string``, it will be checked in the same way. Example 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 index 2bb47d33da389d..faf135833ee15d 100644 --- 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 @@ -17,15 +17,12 @@ struct CustomStringTemplateBase { struct CustomString1 : CustomStringNonTemplateBase {}; struct CustomString2 : CustomStringTemplateBase<char> {}; -void StdClassesAreStillDetected() { +void CustomStringClasses() { std::string_view sv1("a"); std::string_view sv2("b"); - if (sv1.compare(sv2)) { + if (sv1.compare(sv2)) { // No warning - if a std class is not listed in StringLikeClasses, it won't be checked. } - // 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)) { } >From a3427fc5e1b0eff64112205e76daf6e444157689 Mon Sep 17 00:00:00 2001 From: Vadim Dudkin <vvd170...@gmail.com> Date: Mon, 15 Apr 2024 00:36:35 +0300 Subject: [PATCH 4/5] Add storeOptions --- .../clang-tidy/readability/StringCompareCheck.cpp | 5 +++++ .../clang-tidy/readability/StringCompareCheck.h | 1 + 2 files changed, 6 insertions(+) diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp index 0bcf31a4795d2b..5679563d01b7b7 100644 --- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp @@ -33,6 +33,11 @@ StringCompareCheck::StringCompareCheck(StringRef Name, StringLikeClasses(optutils::parseStringList( Options.get("StringLikeClasses", DefaultStringLikeClasses))) {} +void StringCompareCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "StringLikeClasses", + utils::options::serializeStringList(StringLikeClasses)); +} + void StringCompareCheck::registerMatchers(MatchFinder *Finder) { if (StringLikeClasses.empty()) { return; diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h index b7bb5a0097645b..150090901a6e97 100644 --- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h +++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.h @@ -29,6 +29,7 @@ class StringCompareCheck : public ClangTidyCheck { void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; private: const std::vector<StringRef> StringLikeClasses; >From d56bd6a53ce391d3d03859a6ca927e6d970daf54 Mon Sep 17 00:00:00 2001 From: Vadim Dudkin <vvd170...@gmail.com> Date: Mon, 15 Apr 2024 00:38:14 +0300 Subject: [PATCH 5/5] Remove unused include --- clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp index 5679563d01b7b7..f32ed36eac121c 100644 --- a/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/StringCompareCheck.cpp @@ -13,7 +13,6 @@ #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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits