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

[njames93]

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 clang-tidy "modernize-use-default-member-init"/"readability-redundant-string-init" and redundant initializer of std::string

[njames93]

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

250

I'm aware the extra space between the Foo() and the braces is unsightly, but its something that clang-format would fix. Same goes for all other lines

[Eugene.Zelenko]

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

24

Please use static instead of anonymous namespaces. Same below.

26

Could it be const auto *?

149

Could it be const auto *?

161

Please don't use auto when type is not explicitly spelled in same statement or iterator.

[njames93]

Address nits

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

149

This shouldn't be auto anyway. Also the reason its `auto * and not const auto *` is because that's the qualified type getMember returns. But in this example as I'm not modifying I could add a const qualifier

[Eugene.Zelenko]

It'll be reasonable to mention changes in Release Notes (in checks changes section, in alphabetical order).

[njames93]

updated release notes

[njames93]

Checks for string initialisations which are spelt with braces

Ensures the check will detect and fix brace initializations
Example:

std::string a{""};
std::string b = {""};

CxxConstructor : A{""} {}

[njames93]

Tweaked a few documentation issues

[njames93]

whoops typo in arc, will fix, this is another patch

[njames93]

Undid my oopsie sending the wrong patch here

[njames93]

• Updated to trunk, fix release notes

[merge_guards_bot]

Unit tests: pass. 61955 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• Small refactor

[merge_guards_bot]

Unit tests: pass. 62130 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[aaron.ballman]

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

58

Should we be worried about macros here?

It looks a bit like we're ignoring macros entirely for this check, so maybe that can be done as a separate patch instead. The situation I am worried about is:

#if SOMETHING
#define INIT ""
#else
#define INIT "haha"
#endif

std::string S = INIT;

110

Should this also match on something like std::string foo{}; as a redundant init? Similar question for the other cases. (Could be done in a follow-up patch if desired.)

[njames93]

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

58

I feel that everything in this check needs to check macros, but that's probably a follow up patch

110

Probably should, however this is not what this patch is about

[aaron.ballman]

LGTM!

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

58

Okay, separate patch it is.

110

Okay, I'm fine with that as well.

[njames93]

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

[aaron.ballman]

In D72448#1844032, @njames93 wrote:

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

Designed to be macro unsafe, or just didn't consider macros in the first place? I'm not seeing anything that makes me think macros were taken into account, but maybe you're looking at something different from me. Do you have an example of where you think something was designed to be macro unsafe?

[njames93]

In D72448#1844309, @aaron.ballman wrote:

In D72448#1844032, @njames93 wrote:

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

Designed to be macro unsafe, or just didn't consider macros in the first place? I'm not seeing anything that makes me think macros were taken into account, but maybe you're looking at something different from me. Do you have an example of where you think something was designed to be macro unsafe?

This is one of the test cases

#define M(x) x
#define N { std::string s = ""; }
// CHECK-FIXES: #define N { std::string s = ""; }

void h() {
  templ<int>();
  templ<double>();

  M({ std::string s = ""; })
  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
  // CHECK-FIXES: M({ std::string s; })

  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
}
// further down
#define EMPTY_STR ""
void j() {
  std::string a(EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string a;
  std::string b = (EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string b;

Looks like they knew the behaviour they wanted back then

[aaron.ballman]

In D72448#1844320, @njames93 wrote:

In D72448#1844309, @aaron.ballman wrote:

In D72448#1844032, @njames93 wrote:

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

Designed to be macro unsafe, or just didn't consider macros in the first place? I'm not seeing anything that makes me think macros were taken into account, but maybe you're looking at something different from me. Do you have an example of where you think something was designed to be macro unsafe?

This is one of the test cases

#define M(x) x
#define N { std::string s = ""; }
// CHECK-FIXES: #define N { std::string s = ""; }

void h() {
  templ<int>();
  templ<double>();

  M({ std::string s = ""; })
  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
  // CHECK-FIXES: M({ std::string s; })

  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
}
// further down
#define EMPTY_STR ""
void j() {
  std::string a(EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string a;
  std::string b = (EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string b;

Looks like they knew the behaviour they wanted back then

Looking at the source code for the check, this looks more like they were documenting the existing behavior rather than making a design choice. I definitely disagree with some of those fixits. It's your call whether you want to do work in this area or not, but I'm not convinced the original design is what we want today (but finding out more from the code reviews that added the test cases may bring up other information we're not considering right now).

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

 using namespace clang::tidy::matchers;
 
 namespace clang {
 namespace tidy {
 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),
       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()));
+  const auto hasStringTypeName = hasAnyNameStdString(StringNames);
+  const auto hasStringCtorName =
+      hasAnyNameStdString(removeNamespaces(StringNames));
 
   // Match string constructor.
   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(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)));
 
+  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");
+  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
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-dcl51-cpp
   <clang-tidy/checks/cert-dcl51-cpp>` to
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 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
 ^^^^^^^^^^^^^^
 
 
 Improvements to include-fixer
 -----------------------------
 

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

void f() {
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
   // CHECK-FIXES: std::string b;
   std::string c = R"()";
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
   // CHECK-FIXES: std::string c;
   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");
   std::string x = R"(x)";
   std::string y(R"(y)");
   std::string z;
 }
 

 void otherTestStringTests() {
   other::TestString a = "";
   other::TestString b("");
   other::string c = "";
   other::string d("");
   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() {}
+};