Author: Nathan Date: 2020-01-27T23:51:45Z New Revision: 7c90666d2c3cfb5a519275d89195be317e7cc0ab
URL: https://github.com/llvm/llvm-project/commit/7c90666d2c3cfb5a519275d89195be317e7cc0ab DIFF: https://github.com/llvm/llvm-project/commit/7c90666d2c3cfb5a519275d89195be317e7cc0ab.diff LOG: [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 Added: Modified: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp index 6bf0edb7231f..866bb79829d8 100644 --- a/clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp +++ b/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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 196ab3fe8c11..58dbe08343cb 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/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 ^^^^^^^^^^^^^^ 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 index 15df753ece33..c33d1a7d5f2a 100644 --- 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 @@ 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() {} +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
