Skip to content

Commit 7c90666

Browse files
njames93njames93
committed
[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
1 parent c3d20fd commit 7c90666

File tree

3 files changed

+163
-24
lines changed

3 files changed

+163
-24
lines changed

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

Lines changed: 102 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,46 @@ namespace readability {
2020

2121
const char DefaultStringNames[] = "::std::basic_string";
2222

23+
static ast_matchers::internal::Matcher<NamedDecl>
24+
hasAnyNameStdString(std::vector<std::string> Names) {
25+
return ast_matchers::internal::Matcher<NamedDecl>(
26+
new ast_matchers::internal::HasNameMatcher(std::move(Names)));
27+
}
28+
29+
static std::vector<std::string>
30+
removeNamespaces(const std::vector<std::string> &Names) {
31+
std::vector<std::string> Result;
32+
Result.reserve(Names.size());
33+
for (const std::string &Name : Names) {
34+
std::string::size_type ColonPos = Name.rfind(':');
35+
Result.push_back(
36+
Name.substr(ColonPos == std::string::npos ? 0 : ColonPos + 1));
37+
}
38+
return Result;
39+
}
40+
41+
static const CXXConstructExpr *
42+
getConstructExpr(const CXXCtorInitializer &CtorInit) {
43+
const Expr *InitExpr = CtorInit.getInit();
44+
if (const auto *CleanUpExpr = dyn_cast<ExprWithCleanups>(InitExpr))
45+
InitExpr = CleanUpExpr->getSubExpr();
46+
return dyn_cast<CXXConstructExpr>(InitExpr);
47+
}
48+
49+
static llvm::Optional<SourceRange>
50+
getConstructExprArgRange(const CXXConstructExpr &Construct) {
51+
SourceLocation B, E;
52+
for (const Expr *Arg : Construct.arguments()) {
53+
if (B.isInvalid())
54+
B = Arg->getBeginLoc();
55+
if (Arg->getEndLoc().isValid())
56+
E = Arg->getEndLoc();
57+
}
58+
if (B.isInvalid() || E.isInvalid())
59+
return llvm::None;
60+
return SourceRange(B, E);
61+
}
62+
2363
RedundantStringInitCheck::RedundantStringInitCheck(StringRef Name,
2464
ClangTidyContext *Context)
2565
: ClangTidyCheck(Name, Context),
@@ -33,18 +73,9 @@ void RedundantStringInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
3373
void RedundantStringInitCheck::registerMatchers(MatchFinder *Finder) {
3474
if (!getLangOpts().CPlusPlus)
3575
return;
36-
const auto hasStringTypeName = hasAnyName(
37-
SmallVector<StringRef, 3>(StringNames.begin(), StringNames.end()));
38-
39-
// Version of StringNames with namespaces removed
40-
std::vector<std::string> stringNamesNoNamespace;
41-
for (const std::string &name : StringNames) {
42-
std::string::size_type colonPos = name.rfind(':');
43-
stringNamesNoNamespace.push_back(
44-
name.substr(colonPos == std::string::npos ? 0 : colonPos + 1));
45-
}
46-
const auto hasStringCtorName = hasAnyName(SmallVector<StringRef, 3>(
47-
stringNamesNoNamespace.begin(), stringNamesNoNamespace.end()));
76+
const auto hasStringTypeName = hasAnyNameStdString(StringNames);
77+
const auto hasStringCtorName =
78+
hasAnyNameStdString(removeNamespaces(StringNames));
4879

4980
// Match string constructor.
5081
const auto StringConstructorExpr = expr(
@@ -65,29 +96,76 @@ void RedundantStringInitCheck::registerMatchers(MatchFinder *Finder) {
6596
cxxConstructExpr(StringConstructorExpr,
6697
hasArgument(0, ignoringImplicit(EmptyStringCtorExpr)));
6798

99+
const auto StringType = hasType(hasUnqualifiedDesugaredType(
100+
recordType(hasDeclaration(cxxRecordDecl(hasStringTypeName)))));
101+
const auto EmptyStringInit = expr(ignoringImplicit(
102+
anyOf(EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)));
103+
68104
// Match a variable declaration with an empty string literal as initializer.
69105
// Examples:
70106
// string foo = "";
71107
// string bar("");
72108
Finder->addMatcher(
73109
namedDecl(
74-
varDecl(
75-
hasType(hasUnqualifiedDesugaredType(recordType(
76-
hasDeclaration(cxxRecordDecl(hasStringTypeName))))),
77-
hasInitializer(expr(ignoringImplicit(anyOf(
78-
EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)))))
79-
.bind("vardecl"),
110+
varDecl(StringType, hasInitializer(EmptyStringInit)).bind("vardecl"),
80111
unless(parmVarDecl())),
81112
this);
113+
// Match a field declaration with an empty string literal as initializer.
114+
Finder->addMatcher(
115+
namedDecl(fieldDecl(StringType, hasInClassInitializer(EmptyStringInit))
116+
.bind("fieldDecl")),
117+
this);
118+
// Matches Constructor Initializers with an empty string literal as
119+
// initializer.
120+
// Examples:
121+
// Foo() : SomeString("") {}
122+
Finder->addMatcher(
123+
cxxCtorInitializer(
124+
isWritten(),
125+
forField(allOf(StringType, optionally(hasInClassInitializer(
126+
EmptyStringInit.bind("empty_init"))))),
127+
withInitializer(EmptyStringInit))
128+
.bind("ctorInit"),
129+
this);
82130
}
83131

84132
void RedundantStringInitCheck::check(const MatchFinder::MatchResult &Result) {
85-
const auto *VDecl = Result.Nodes.getNodeAs<VarDecl>("vardecl");
86-
// VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
87-
// So start at getLocation() to span just 'foo = ""' or 'bar("")'.
88-
SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
89-
diag(VDecl->getLocation(), "redundant string initialization")
90-
<< FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
133+
if (const auto *VDecl = Result.Nodes.getNodeAs<VarDecl>("vardecl")) {
134+
// VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
135+
// So start at getLocation() to span just 'foo = ""' or 'bar("")'.
136+
SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
137+
diag(VDecl->getLocation(), "redundant string initialization")
138+
<< FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
139+
}
140+
if (const auto *FDecl = Result.Nodes.getNodeAs<FieldDecl>("fieldDecl")) {
141+
// FieldDecl's getSourceRange() spans 'string foo = ""'.
142+
// So start at getLocation() to span just 'foo = ""'.
143+
SourceRange ReplaceRange(FDecl->getLocation(), FDecl->getEndLoc());
144+
diag(FDecl->getLocation(), "redundant string initialization")
145+
<< FixItHint::CreateReplacement(ReplaceRange, FDecl->getName());
146+
}
147+
if (const auto *CtorInit =
148+
Result.Nodes.getNodeAs<CXXCtorInitializer>("ctorInit")) {
149+
if (const FieldDecl *Member = CtorInit->getMember()) {
150+
if (!Member->hasInClassInitializer() ||
151+
Result.Nodes.getNodeAs<Expr>("empty_init")) {
152+
// The String isn't declared in the class with an initializer or its
153+
// declared with a redundant initializer, which will be removed. Either
154+
// way the string will be default initialized, therefore we can remove
155+
// the constructor initializer entirely.
156+
diag(CtorInit->getMemberLocation(), "redundant string initialization")
157+
<< FixItHint::CreateRemoval(CtorInit->getSourceRange());
158+
return;
159+
}
160+
}
161+
const CXXConstructExpr *Construct = getConstructExpr(*CtorInit);
162+
if (!Construct)
163+
return;
164+
if (llvm::Optional<SourceRange> RemovalRange =
165+
getConstructExprArgRange(*Construct))
166+
diag(CtorInit->getMemberLocation(), "redundant string initialization")
167+
<< FixItHint::CreateRemoval(*RemovalRange);
168+
}
91169
}
92170

93171
} // namespace readability

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,11 @@ New aliases
104104
Changes in existing checks
105105
^^^^^^^^^^^^^^^^^^^^^^^^^^
106106

107+
- Improved :doc:`readability-redundant-string-init
108+
<clang-tidy/checks/readability-redundant-string-init>` check now supports a
109+
`StringNames` option enabling its application to custom string classes. The
110+
check now detects in class initializers and constructor initializers which
111+
are deemed to be redundant.
107112

108113
Renamed checks
109114
^^^^^^^^^^^^^^

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ void f() {
3434
std::string d(R"()");
3535
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
3636
// CHECK-FIXES: std::string d;
37+
std::string e{""};
38+
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
39+
// CHECK-FIXES: std::string e;
40+
std::string f = {""};
41+
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
42+
// CHECK-FIXES: std::string f;
3743

3844
std::string u = "u";
3945
std::string w("w");
@@ -227,3 +233,53 @@ void otherTestStringTests() {
227233
other::wstring e = L"";
228234
other::wstring f(L"");
229235
}
236+
237+
class Foo {
238+
std::string A = "";
239+
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
240+
// CHECK-FIXES: std::string A;
241+
std::string B;
242+
std::string C;
243+
std::string D;
244+
std::string E = "NotEmpty";
245+
246+
public:
247+
// Check redundant constructor where Field has a redundant initializer.
248+
Foo() : A("") {}
249+
// CHECK-MESSAGES: [[@LINE-1]]:11: warning: redundant string initialization
250+
// CHECK-FIXES: Foo() {}
251+
252+
// Check redundant constructor where Field has no initializer.
253+
Foo(char) : B("") {}
254+
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
255+
// CHECK-FIXES: Foo(char) {}
256+
257+
// Check redundant constructor where Field has a valid initializer.
258+
Foo(long) : E("") {}
259+
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
260+
// CHECK-FIXES: Foo(long) : E() {}
261+
262+
// Check how it handles removing 1 initializer, and defaulting the other.
263+
Foo(int) : B(""), E("") {}
264+
// CHECK-MESSAGES: [[@LINE-1]]:14: warning: redundant string initialization
265+
// CHECK-MESSAGES: [[@LINE-2]]:21: warning: redundant string initialization
266+
// CHECK-FIXES: Foo(int) : E() {}
267+
268+
Foo(short) : B{""} {}
269+
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: redundant string initialization
270+
// CHECK-FIXES: Foo(short) {}
271+
272+
Foo(float) : A{""}, B{""} {}
273+
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: redundant string initialization
274+
// CHECK-MESSAGES: [[@LINE-2]]:23: warning: redundant string initialization
275+
// CHECK-FIXES: Foo(float) {}
276+
277+
278+
// Check how it handles removing some redundant initializers while leaving
279+
// valid initializers intact.
280+
Foo(std::string Arg) : A(Arg), B(""), C("NonEmpty"), D(R"()"), E("") {}
281+
// CHECK-MESSAGES: [[@LINE-1]]:34: warning: redundant string initialization
282+
// CHECK-MESSAGES: [[@LINE-2]]:56: warning: redundant string initialization
283+
// CHECK-MESSAGES: [[@LINE-3]]:66: warning: redundant string initialization
284+
// CHECK-FIXES: Foo(std::string Arg) : A(Arg), C("NonEmpty"), E() {}
285+
};

0 commit comments

Comments
 (0)