Give clang-tidy readability-redundant-string-init a customizable list of string types to fix

[poelmanc]

Summary

This patch adds a feature requested by @MyDeveloperDay at https://reviews.llvm.org/D69238 to enable readability-redundant-string-init to take a list of strings to apply the fix to rather than hard-coding basic_string. It adds a StringNames option of semicolon-delimited names of string classes to which to apply this fix. Tests ensure this works with test class out::TestString as well as std::string and std::wstring as before. It should be applicable to llvm::StringRef, QString, etc.

[Eugene.Zelenko]

Please mention new option in Release Notes (in new options section, if there is no such yet, see previous release for order of sections).

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

15

Please remove empty line.

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

27

Please use single back-ticks for options. Same below.

[MyDeveloperDay]

Thanks you so much for this patch, I obviously like it! So many of us who write our own string classes we tend to make them have similar interfaces to std::string anyway for obvious reasons (some of us actually add decent startWith,endsWith and contains functions!), so I think this is a great addition... I'm gonna mention that there are other checks that are pertinent, but let us see if this one gets accepted first. You get my LGTM, but let's run it by the code owners.

[aaron.ballman]

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

21

I think the default should probably be ::std::basic_string to avoid getting other things named basic_string?

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

27

Should update this as well if you go with the suggest change to ::std::basic_string.

clang/include/clang/ASTMatchers/ASTMatchers.h

2563–2565 ↗

(On Diff #226836)

I think we should try to get the existing hasAnyName() matcher to work with a vector of strings instead of adding a new matcher to do the same thing.

[poelmanc]

Added release notes, fixed backticks in documentation, removed blank line, removed new hasListedName matcher and used existing hasAnyName matcher.

[poelmanc]

@aaron.ballman Thanks for the hasAnyName feedback! From the name internal::VariadicFunction I assumed arguments were needed at compile-time, but thanks to your suggestion I found the overload taking ArrayRef<ArgT>.

[poelmanc]

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

21

The prior code had basic_string hard-coded at lines 27, 31, and 50 so I initially set the default to basic_string just to keep the default behavior unchanged.

I just now tried setting it to each of std::basic_string, ::std::basic_string, and even std::string;std::wstring and the readability-redundant-string-init.cpp tests failed with any of those settings, so I've left it set to basic_string.

[gribozavr2]

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

21

I think we should change the checker behavior so that it checks against the fully-qualified name.

We are defining a user-settable option, and changing its behavior later to allow fully-qualified names could be difficult.

The reason why tests failed for you is because below the name "basic_string" is used as a method name (the name of the constructor). That checker should be rewritten to check the name of the type.

[poelmanc]

Now allows namespaces on types and defaults to ::std::basic_string as requested. The code uses namespaced string type names to check types, and uses non-namespaced string type names to check for the required one-argument or two-argument-defaulted constructors.

[gribozavr2]

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

29

"of class names"

I don't think there's anything about base classes here.

30

Please wrap std::string and std::wstring in backtics.

[Eugene.Zelenko]

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

30

Double back-ticks, because names are language constructs here.

[poelmanc]

Make requested fixes to documentation.

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

 //===- RedundantStringInitCheck.h - clang-tidy ------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_STRING_INIT_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_STRING_INIT_H
 
 #include "../ClangTidyCheck.h"
+#include <string>
+#include <vector>
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 /// Finds unnecessary string initializations.
 class RedundantStringInitCheck : public ClangTidyCheck {
 public:
-  RedundantStringInitCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  RedundantStringInitCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::vector<std::string> StringNames;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_STRING_INIT_H

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

 //===- RedundantStringInitCheck.cpp - clang-tidy ----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "RedundantStringInitCheck.h"
 #include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tidy::matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+const char DefaultStringNames[] = "::std::basic_string";
+
+RedundantStringInitCheck::RedundantStringInitCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StringNames(utils::options::parseStringList(
+          Options.get("StringNames", DefaultStringNames))) {}
+
+void RedundantStringInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "StringNames", DefaultStringNames);
+}
+
 void RedundantStringInitCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
+  const auto hasStringTypeName = hasAnyName(
+      SmallVector<StringRef, 3>(StringNames.begin(), StringNames.end()));
+
+  // Version of StringNames with namespaces removed
+  std::vector<std::string> stringNamesNoNamespace;
+  for (const std::string &name : StringNames) {
+    std::string::size_type colonPos = name.rfind(':');
+    stringNamesNoNamespace.push_back(
+        name.substr(colonPos == std::string::npos ? 0 : colonPos + 1));
+  }
+  const auto hasStringCtorName = hasAnyName(SmallVector<StringRef, 3>(
+      stringNamesNoNamespace.begin(), stringNamesNoNamespace.end()));
 
   // Match string constructor.
-  const auto StringConstructorExpr = expr(anyOf(
-      cxxConstructExpr(argumentCountIs(1),
-                       hasDeclaration(cxxMethodDecl(hasName("basic_string")))),
-      // If present, the second argument is the alloc object which must not
-      // be present explicitly.
+  const auto StringConstructorExpr = expr(
+      anyOf(cxxConstructExpr(argumentCountIs(1),
+                             hasDeclaration(cxxMethodDecl(hasStringCtorName))),
+            // If present, the second argument is the alloc object which must
+            // not be present explicitly.
             cxxConstructExpr(argumentCountIs(2),
-                       hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+                             hasDeclaration(cxxMethodDecl(hasStringCtorName)),
                              hasArgument(1, cxxDefaultArgExpr()))));
 
   // Match a string constructor expression with an empty string literal.
   const auto EmptyStringCtorExpr = cxxConstructExpr(
       StringConstructorExpr,
       hasArgument(0, ignoringParenImpCasts(stringLiteral(hasSize(0)))));
 
   const auto EmptyStringCtorExprWithTemporaries =
       cxxConstructExpr(StringConstructorExpr,
                        hasArgument(0, ignoringImplicit(EmptyStringCtorExpr)));
 
   // Match a variable declaration with an empty string literal as initializer.
   // Examples:
   //     string foo = "";
   //     string bar("");
   Finder->addMatcher(
       namedDecl(
           varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
-                      hasDeclaration(cxxRecordDecl(hasName("basic_string")))))),
+                      hasDeclaration(cxxRecordDecl(hasStringTypeName))))),
                   hasInitializer(expr(ignoringImplicit(anyOf(
                                           EmptyStringCtorExpr,
                                           EmptyStringCtorExprWithTemporaries)))
                                      .bind("expr"))),
           unless(parmVarDecl()))
           .bind("decl"),
       this);
 }

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.
 
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
 - The 'objc-avoid-spinlock' check was renamed to :doc:`darwin-avoid-spinlock
   <clang-tidy/checks/darwin-avoid-spinlock>`
 
+- The :doc:`readability-redundant-string-init
+  <clang-tidy/checks/readability-redundant-string-init>` check now supports a
+  `StringNames` option enabling its application to custom string classes.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

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

 .. title:: clang-tidy - readability-redundant-string-init
 
 readability-redundant-string-init
 =================================
 
 Finds unnecessary string initializations.
 
-Examples:
+Examples
+--------
 
 .. code-block:: c++
 
   // Initializing string with empty string literal is unnecessary.
   std::string a = "";
   std::string b("");
 
   // becomes
 
   std::string a;
   std::string b;
+
+Options
+-------
+
+.. option:: StringNames
+
+    Default is `::std::basic_string`.
+
+    Semicolon-delimited list of base class names to apply this check to.
+    By default `::std::basic_string` applies to std::string and std::wstring.
+    Set to e.g. `::std::basic_string;llvm::StringRef;QString` to perform this
+    check on custom classes.

clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp

-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
+// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: readability-redundant-string-init.StringNames, \
+// RUN:               value: "::std::basic_string;our::TestString"}] \
+// RUN:             }"
 // FIXME: Fix the checker to work in C++17 mode.
 
 namespace std {
 template <typename T>
 class allocator {};
 template <typename T>
 class char_traits {};
 template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>

 }
 
 // These cases should not generate warnings.
 extern void Param1(std::string param = "");
 extern void Param2(const std::string& param = "");
 void Param3(std::string param = "") {}
 void Param4(STRING param = "") {}
 
+namespace our {
+struct TestString {
+  TestString();
+  TestString(const TestString &);
+  TestString(const char *);
+  ~TestString();
+};
+}
+
+void ourTestStringTests() {
+  our::TestString a = "";
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
+  // CHECK-FIXES: our::TestString a;
+  our::TestString b("");
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
+  // CHECK-FIXES: our::TestString b;
+  our::TestString c = R"()";
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
+  // CHECK-FIXES: our::TestString c;
+  our::TestString d(R"()");
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
+  // CHECK-FIXES: our::TestString d;
+
+  our::TestString u = "u";
+  our::TestString w("w");
+  our::TestString x = R"(x)";
+  our::TestString y(R"(y)");
+  our::TestString z;
+}
+
+namespace their {
+using TestString = our::TestString;
+}
+
+// their::TestString is the same type so should warn / fix
+void theirTestStringTests() {
+  their::TestString a = "";
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: redundant string initialization
+  // CHECK-FIXES: their::TestString a;
+  their::TestString b("");
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: redundant string initialization
+  // CHECK-FIXES: their::TestString b;
+}
+
+namespace other {
+// Identical declarations to above but different type
+struct TestString {
+  TestString();
+  TestString(const TestString &);
+  TestString(const char *);
+  ~TestString();
+};
+
+// Identical declarations to above but different type
+template <typename T>
+class allocator {};
+template <typename T>
+class char_traits {};
+template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C>>
+struct basic_string {
+  basic_string();
+  basic_string(const basic_string &);
+  basic_string(const C *, const A &a = A());
+  ~basic_string();
+};
+typedef basic_string<char> string;
+typedef basic_string<wchar_t> wstring;
+}
+
+// other::TestString, other::string, other::wstring are unrelated to the types
+// being checked. No warnings / fixes should be produced for these types.
+void otherTestStringTests() {
+  other::TestString a = "";
+  other::TestString b("");
+  other::string c = "";
+  other::string d("");
+  other::wstring e = L"";
+  other::wstring f(L"");
+}