Skip to content

Commit a37b76a

Browse files
committed
fixup! [clang-tidy] performance-unnecessary-copy-init
1 parent 851460a commit a37b76a

File tree

2 files changed

+85
-63
lines changed

2 files changed

+85
-63
lines changed

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

Lines changed: 61 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -262,21 +262,27 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
262262
this);
263263
}
264264

265+
UnnecessaryCopyInitialization::CheckContext::CheckContext(
266+
const ast_matchers::MatchFinder::MatchResult &Result)
267+
: Var(*Result.Nodes.getNodeAs<VarDecl>("newVarDecl")),
268+
BlockStmt(*Result.Nodes.getNodeAs<Stmt>("blockStmt")),
269+
VarDeclStmt(*Result.Nodes.getNodeAs<DeclStmt>("declStmt")),
270+
ASTCtx(*Result.Context),
271+
// Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
272+
// since we cannot place them correctly.
273+
IssueFix(VarDeclStmt.isSingleDecl() && !Var.getLocation().isMacroID()),
274+
IsVarUnused(isVariableUnused(Var, BlockStmt, ASTCtx)),
275+
IsVarOnlyUsedAsConst(isOnlyUsedAsConst(Var, BlockStmt, ASTCtx)) {}
276+
265277
void UnnecessaryCopyInitialization::check(
266278
const MatchFinder::MatchResult &Result) {
267-
const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
279+
const CheckContext Context(Result);
268280
const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>(OldVarDeclId);
269281
const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>(ObjectArgId);
270-
const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
271282
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
272-
const auto *Stmt = Result.Nodes.getNodeAs<DeclStmt>("declStmt");
273283

274284
TraversalKindScope RAII(*Result.Context, TK_AsIs);
275285

276-
// Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
277-
// since we cannot place them correctly.
278-
bool IssueFix = Stmt->isSingleDecl() && !NewVar->getLocation().isMacroID();
279-
280286
// A constructor that looks like T(const T& t, bool arg = false) counts as a
281287
// copy only when it is called with default arguments for the arguments after
282288
// the first.
@@ -290,69 +296,72 @@ void UnnecessaryCopyInitialization::check(
290296
// instantiations where the types differ and rely on implicit conversion would
291297
// no longer compile if we switched to a reference.
292298
if (differentReplacedTemplateParams(
293-
NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes),
299+
Context.Var.getType(), constructorArgumentType(OldVar, Result.Nodes),
294300
*Result.Context))
295301
return;
296302

297303
if (OldVar == nullptr) {
298-
handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
299-
*Result.Context);
304+
// `auto NewVar = functionCall();`
305+
handleCopyFromMethodReturn(Context, ObjectArg);
300306
} else {
301-
handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
302-
*Result.Context);
307+
// `auto NewVar = OldVar;`
308+
handleCopyFromLocalVar(Context, *OldVar);
303309
}
304310
}
305311

306-
void UnnecessaryCopyInitialization::makeDiagnostic(
307-
DiagnosticBuilder Diagnostic, const VarDecl &Var, const Stmt &BlockStmt,
308-
const DeclStmt &Stmt, ASTContext &Context, bool IssueFix) {
309-
const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
310-
Diagnostic << &Var << IsVarUnused;
311-
if (!IssueFix)
312-
return;
313-
if (IsVarUnused)
314-
recordRemoval(Stmt, Context, Diagnostic);
315-
else
316-
recordFixes(Var, Context, Diagnostic);
317-
}
318-
319312
void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
320-
const VarDecl &Var, const Stmt &BlockStmt, const DeclStmt &Stmt,
321-
bool IssueFix, const VarDecl *ObjectArg, ASTContext &Context) {
322-
bool IsConstQualified = Var.getType().isConstQualified();
323-
if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
313+
const CheckContext &Ctx, const VarDecl *ObjectArg) {
314+
bool IsConstQualified = Ctx.Var.getType().isConstQualified();
315+
if (!IsConstQualified && !Ctx.IsVarOnlyUsedAsConst)
324316
return;
325317
if (ObjectArg != nullptr &&
326-
!isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
318+
!isInitializingVariableImmutable(*ObjectArg, Ctx.BlockStmt, Ctx.ASTCtx,
327319
ExcludedContainerTypes))
328320
return;
329-
330-
auto Diagnostic =
331-
diag(Var.getLocation(),
332-
"the %select{|const qualified }0variable %1 is copy-constructed "
333-
"from a const reference%select{"
334-
"%select{ but is only used as const reference|}0"
335-
"| but is never used}2; consider "
336-
"%select{making it a const reference|removing the statement}2")
337-
<< IsConstQualified;
338-
makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
339-
IssueFix);
321+
diagnoseCopyFromMethodReturn(Ctx, ObjectArg);
340322
}
341323

342324
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
343-
const VarDecl &Var, const VarDecl &OldVar, const Stmt &BlockStmt,
344-
const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
345-
if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
346-
!isInitializingVariableImmutable(OldVar, BlockStmt, Context,
325+
const CheckContext &Ctx, const VarDecl &OldVar) {
326+
if (!Ctx.IsVarOnlyUsedAsConst ||
327+
!isInitializingVariableImmutable(OldVar, Ctx.BlockStmt, Ctx.ASTCtx,
347328
ExcludedContainerTypes))
348329
return;
349-
auto Diagnostic = diag(Var.getLocation(),
350-
"local copy %1 of the variable %0 is never modified"
351-
"%select{| and never used}2; consider "
352-
"%select{avoiding the copy|removing the statement}2")
353-
<< &OldVar;
354-
makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
355-
IssueFix);
330+
diagnoseCopyFromLocalVar(Ctx, OldVar);
331+
}
332+
333+
void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
334+
const CheckContext &Ctx, const VarDecl *ObjectArg) {
335+
auto Diagnostic =
336+
diag(Ctx.Var.getLocation(),
337+
"the %select{|const qualified }0variable %1 is "
338+
"copy-constructed "
339+
"from a const reference%select{%select{ but is only used as const "
340+
"reference|}0| but is never used}2; consider "
341+
"%select{making it a const reference|removing the statement}2")
342+
<< Ctx.Var.getType().isConstQualified() << &Ctx.Var << Ctx.IsVarUnused;
343+
maybeIssueFixes(Ctx, Diagnostic);
344+
}
345+
void UnnecessaryCopyInitialization::diagnoseCopyFromLocalVar(
346+
const CheckContext &Ctx, const VarDecl &OldVar) {
347+
auto Diagnostic =
348+
diag(Ctx.Var.getLocation(),
349+
"local copy %1 of the variable %0 is never modified%select{"
350+
"| and never used}2; consider %select{avoiding the copy|removing "
351+
"the statement}2")
352+
<< &OldVar << &Ctx.Var << Ctx.IsVarUnused;
353+
maybeIssueFixes(Ctx, Diagnostic);
354+
}
355+
356+
void UnnecessaryCopyInitialization::maybeIssueFixes(
357+
const CheckContext &Ctx, DiagnosticBuilder &Diagnostic) {
358+
if (Ctx.IssueFix) {
359+
if (Ctx.IsVarUnused) {
360+
recordRemoval(Ctx.VarDeclStmt, Ctx.ASTCtx, Diagnostic);
361+
} else {
362+
recordFixes(Ctx.Var, Ctx.ASTCtx, Diagnostic);
363+
}
364+
}
356365
}
357366

358367
void UnnecessaryCopyInitialization::storeOptions(

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,32 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
3333
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
3434

3535
protected:
36-
// This is virtual so that derived classes can implement additional behavior.
37-
virtual void makeDiagnostic(DiagnosticBuilder Diagnostic, const VarDecl &Var,
38-
const Stmt &BlockStmt, const DeclStmt &Stmt,
39-
ASTContext &Context, bool IssueFix);
36+
// A helper to manipulate the state common to
37+
// `CopyFromMethodReturn` and `CopyFromLocalVar`.
38+
struct CheckContext {
39+
CheckContext(const ast_matchers::MatchFinder::MatchResult &Result);
40+
const VarDecl &Var;
41+
const Stmt &BlockStmt;
42+
const DeclStmt &VarDeclStmt;
43+
clang::ASTContext &ASTCtx;
44+
const bool IssueFix;
45+
const bool IsVarUnused;
46+
const bool IsVarOnlyUsedAsConst;
47+
};
48+
49+
// Create diagnostics. These are virtual so that derived classes can change
50+
// behaviour.
51+
virtual void diagnoseCopyFromMethodReturn(const CheckContext &Ctx,
52+
const VarDecl *ObjectArg);
53+
virtual void diagnoseCopyFromLocalVar(const CheckContext &Ctx,
54+
const VarDecl &OldVar);
4055

4156
private:
42-
void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
43-
const DeclStmt &Stmt, bool IssueFix,
44-
const VarDecl *ObjectArg,
45-
ASTContext &Context);
46-
void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
47-
const Stmt &BlockStmt, const DeclStmt &Stmt,
48-
bool IssueFix, ASTContext &Context);
57+
void handleCopyFromMethodReturn(const CheckContext &Ctx,
58+
const VarDecl *ObjectArg);
59+
void handleCopyFromLocalVar(const CheckContext &Ctx, const VarDecl &OldVar);
60+
61+
void maybeIssueFixes(const CheckContext &Ctx, DiagnosticBuilder &Diagnostic);
4962

5063
const std::vector<StringRef> AllowedTypes;
5164
const std::vector<StringRef> ExcludedContainerTypes;

0 commit comments

Comments
 (0)