diff --git a/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp @@ -20,6 +20,46 @@ const char DefaultStringNames[] = "::std::basic_string"; +static ast_matchers::internal::Matcher +hasAnyNameStdString(std::vector Names) { + return ast_matchers::internal::Matcher( + new ast_matchers::internal::HasNameMatcher(std::move(Names))); +} + +static std::vector +removeNamespaces(const std::vector &Names) { + std::vector 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(InitExpr)) + InitExpr = CleanUpExpr->getSubExpr(); + return dyn_cast(InitExpr); +} + +static llvm::Optional +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::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus) return; - const auto hasStringTypeName = hasAnyName( - SmallVector(StringNames.begin(), StringNames.end())); - - // Version of StringNames with namespaces removed - std::vector 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( - 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 @@ 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'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'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'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("ctorInit")) { + if (const FieldDecl *Member = CtorInit->getMember()) { + if (!Member->hasInClassInitializer() || + Result.Nodes.getNodeAs("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 RemovalRange = + getConstructExprArgRange(*Construct)) + diag(CtorInit->getMemberLocation(), "redundant string initialization") + << FixItHint::CreateRemoval(*RemovalRange); + } } } // namespace readability diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,11 @@ Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`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 ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp @@ -34,6 +34,12 @@ 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 @@ 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() {} +};