[clang-tidy] readability-redundant-string-init now flags redundant initialisation in Field Decls and Constructor Initialisers

Summary:
The original behaviour of this check only looked at VarDecls with strings that had an empty string initializer. This has been improved to check for FieldDecls with an in class initializer as well as constructor initializers.

Addresses [[ https://bugs.llvm.org/show_bug.cgi?id=44474 | clang-tidy "modernize-use-default-member-init"/"readability-redundant-string-init" and redundant initializer of std::string ]]

Reviewers: aaron.ballman, alexfh, hokein

Reviewed By: aaron.ballman

Subscribers: merge_guards_bot, mgorny, Eugene.Zelenko, xazax.hun, cfe-commits

Tags: #clang, #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D72448

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

@@ -20,6 +20,46 @@ namespace readability {
 
 const char DefaultStringNames[] = "::std::basic_string";
 
+static ast_matchers::internal::Matcher<NamedDecl>
+hasAnyNameStdString(std::vector<std::string> Names) {
+  return ast_matchers::internal::Matcher<NamedDecl>(
+      new ast_matchers::internal::HasNameMatcher(std::move(Names)));
+}
+
+static std::vector<std::string>
+removeNamespaces(const std::vector<std::string> &Names) {
+  std::vector<std::string> Result;
+  Result.reserve(Names.size());
+  for (const std::string &Name : Names) {
+    std::string::size_type ColonPos = Name.rfind(':');
+    Result.push_back(
+        Name.substr(ColonPos == std::string::npos ? 0 : ColonPos + 1));
+  }
+  return Result;
+}
+
+static const CXXConstructExpr *
+getConstructExpr(const CXXCtorInitializer &CtorInit) {
+  const Expr *InitExpr = CtorInit.getInit();
+  if (const auto *CleanUpExpr = dyn_cast<ExprWithCleanups>(InitExpr))
+    InitExpr = CleanUpExpr->getSubExpr();
+  return dyn_cast<CXXConstructExpr>(InitExpr);
+}
+
+static llvm::Optional<SourceRange>
+getConstructExprArgRange(const CXXConstructExpr &Construct) {
+  SourceLocation B, E;
+  for (const Expr *Arg : Construct.arguments()) {
+    if (B.isInvalid())
+      B = Arg->getBeginLoc();
+    if (Arg->getEndLoc().isValid())
+      E = Arg->getEndLoc();
+  }
+  if (B.isInvalid() || E.isInvalid())
+    return llvm::None;
+  return SourceRange(B, E);
+}
+
 RedundantStringInitCheck::RedundantStringInitCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
@@ -33,18 +73,9 @@ void RedundantStringInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 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()));
+  const auto hasStringTypeName = hasAnyNameStdString(StringNames);
+  const auto hasStringCtorName =
+      hasAnyNameStdString(removeNamespaces(StringNames));
 
   // Match string constructor.
   const auto StringConstructorExpr = expr(
@@ -65,29 +96,76 @@ void RedundantStringInitCheck::registerMatchers(MatchFinder *Finder) {
       cxxConstructExpr(StringConstructorExpr,
                        hasArgument(0, ignoringImplicit(EmptyStringCtorExpr)));
 
+  const auto StringType = hasType(hasUnqualifiedDesugaredType(
+      recordType(hasDeclaration(cxxRecordDecl(hasStringTypeName)))));
+  const auto EmptyStringInit = expr(ignoringImplicit(
+      anyOf(EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)));
+
   // 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(hasStringTypeName))))),
-              hasInitializer(expr(ignoringImplicit(anyOf(
-                  EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)))))
-              .bind("vardecl"),
+          varDecl(StringType, hasInitializer(EmptyStringInit)).bind("vardecl"),
           unless(parmVarDecl())),
       this);
+  // Match a field declaration with an empty string literal as initializer.
+  Finder->addMatcher(
+      namedDecl(fieldDecl(StringType, hasInClassInitializer(EmptyStringInit))
+                    .bind("fieldDecl")),
+      this);
+  // Matches Constructor Initializers with an empty string literal as
+  // initializer.
+  // Examples:
+  //     Foo() : SomeString("") {}
+  Finder->addMatcher(
+      cxxCtorInitializer(
+          isWritten(),
+          forField(allOf(StringType, optionally(hasInClassInitializer(
+                                         EmptyStringInit.bind("empty_init"))))),
+          withInitializer(EmptyStringInit))
+          .bind("ctorInit"),
+      this);
 }
 
 void RedundantStringInitCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *VDecl = Result.Nodes.getNodeAs<VarDecl>("vardecl");
-  // VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
-  // So start at getLocation() to span just 'foo = ""' or 'bar("")'.
-  SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
-  diag(VDecl->getLocation(), "redundant string initialization")
-      << FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
+  if (const auto *VDecl = Result.Nodes.getNodeAs<VarDecl>("vardecl")) {
+    // VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
+    // So start at getLocation() to span just 'foo = ""' or 'bar("")'.
+    SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
+    diag(VDecl->getLocation(), "redundant string initialization")
+        << FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
+  }
+  if (const auto *FDecl = Result.Nodes.getNodeAs<FieldDecl>("fieldDecl")) {
+    // FieldDecl's getSourceRange() spans 'string foo = ""'.
+    // So start at getLocation() to span just 'foo = ""'.
+    SourceRange ReplaceRange(FDecl->getLocation(), FDecl->getEndLoc());
+    diag(FDecl->getLocation(), "redundant string initialization")
+        << FixItHint::CreateReplacement(ReplaceRange, FDecl->getName());
+  }
+  if (const auto *CtorInit =
+          Result.Nodes.getNodeAs<CXXCtorInitializer>("ctorInit")) {
+    if (const FieldDecl *Member = CtorInit->getMember()) {
+      if (!Member->hasInClassInitializer() ||
+          Result.Nodes.getNodeAs<Expr>("empty_init")) {
+        // The String isn't declared in the class with an initializer or its
+        // declared with a redundant initializer, which will be removed. Either
+        // way the string will be default initialized, therefore we can remove
+        // the constructor initializer entirely.
+        diag(CtorInit->getMemberLocation(), "redundant string initialization")
+            << FixItHint::CreateRemoval(CtorInit->getSourceRange());
+        return;
+      }
+    }
+    const CXXConstructExpr *Construct = getConstructExpr(*CtorInit);
+    if (!Construct)
+      return;
+    if (llvm::Optional<SourceRange> RemovalRange =
+            getConstructExprArgRange(*Construct))
+      diag(CtorInit->getMemberLocation(), "redundant string initialization")
+          << FixItHint::CreateRemoval(*RemovalRange);
+  }
 }
 
 } // namespace readability

clang-tools-extra/docs/ReleaseNotes.rst

@@ -104,6 +104,11 @@ New aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :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. The 
+  check now detects in class initializers and constructor initializers which 
+  are deemed to be redundant.
 
 Renamed checks
 ^^^^^^^^^^^^^^

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

@@ -34,6 +34,12 @@ void f() {
   std::string d(R"()");
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
   // CHECK-FIXES: std::string d;
+  std::string e{""};
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-FIXES: std::string e;
+  std::string f = {""};
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-FIXES: std::string f;
 
   std::string u = "u";
   std::string w("w");
@@ -227,3 +233,53 @@ void otherTestStringTests() {
   other::wstring e = L"";
   other::wstring f(L"");
 }
+
+class Foo {
+  std::string A = "";
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-FIXES:  std::string A;
+  std::string B;
+  std::string C;
+  std::string D;
+  std::string E = "NotEmpty";
+
+public:
+  // Check redundant constructor where Field has a redundant initializer.
+  Foo() : A("") {}
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: redundant string initialization
+  // CHECK-FIXES:  Foo()  {}
+
+  // Check redundant constructor where Field has no initializer.
+  Foo(char) : B("") {}
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-FIXES:  Foo(char)  {}
+
+  // Check redundant constructor where Field has a valid initializer.
+  Foo(long) : E("") {}
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
+  // CHECK-FIXES:  Foo(long) : E() {}
+
+  // Check how it handles removing 1 initializer, and defaulting the other.
+  Foo(int) : B(""), E("") {}
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-2]]:21: warning: redundant string initialization
+  // CHECK-FIXES:  Foo(int) :  E() {}
+
+  Foo(short) : B{""} {}
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: redundant string initialization
+  // CHECK-FIXES:  Foo(short)  {}
+
+  Foo(float) : A{""}, B{""} {}
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: redundant string initialization
+  // CHECK-FIXES:  Foo(float)  {}
+
+
+  // Check how it handles removing some redundant initializers while leaving
+  // valid initializers intact.
+  Foo(std::string Arg) : A(Arg), B(""), C("NonEmpty"), D(R"()"), E("") {}
+  // CHECK-MESSAGES: [[@LINE-1]]:34: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-2]]:56: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-3]]:66: warning: redundant string initialization
+  // CHECK-FIXES:  Foo(std::string Arg) : A(Arg),  C("NonEmpty"),  E() {}
+};