Skip to content

Commit 04ce9a3

Browse files
authored
[clang-tidy] performance-unnecessary-copy-init: Add a hook... (#73921)
... so that derived checks can can observe for which variables a warning has been emitted. Does nothing by default, which makes this an NFC.
1 parent fc42a2f commit 04ce9a3

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: 60 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "clang/AST/Decl.h"
1616
#include "clang/Basic/Diagnostic.h"
1717
#include <optional>
18+
#include <utility>
1819

1920
namespace clang::tidy::performance {
2021
namespace {
@@ -263,19 +264,25 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
263264

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

273284
TraversalKindScope RAII(*Result.Context, TK_AsIs);
274285

275-
// Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
276-
// since we cannot place them correctly.
277-
bool IssueFix = Stmt->isSingleDecl() && !NewVar->getLocation().isMacroID();
278-
279286
// A constructor that looks like T(const T& t, bool arg = false) counts as a
280287
// copy only when it is called with default arguments for the arguments after
281288
// the first.
@@ -289,74 +296,71 @@ void UnnecessaryCopyInitialization::check(
289296
// instantiations where the types differ and rely on implicit conversion would
290297
// no longer compile if we switched to a reference.
291298
if (differentReplacedTemplateParams(
292-
NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes),
299+
Context.Var.getType(), constructorArgumentType(OldVar, Result.Nodes),
293300
*Result.Context))
294301
return;
295302

296303
if (OldVar == nullptr) {
297-
handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
298-
*Result.Context);
304+
// `auto NewVar = functionCall();`
305+
handleCopyFromMethodReturn(Context, ObjectArg);
299306
} else {
300-
handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
301-
*Result.Context);
307+
// `auto NewVar = OldVar;`
308+
handleCopyFromLocalVar(Context, *OldVar);
302309
}
303310
}
304311

305312
void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
306-
const VarDecl &Var, const Stmt &BlockStmt, const DeclStmt &Stmt,
307-
bool IssueFix, const VarDecl *ObjectArg, ASTContext &Context) {
308-
bool IsConstQualified = Var.getType().isConstQualified();
309-
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)
310316
return;
311317
if (ObjectArg != nullptr &&
312-
!isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
318+
!isInitializingVariableImmutable(*ObjectArg, Ctx.BlockStmt, Ctx.ASTCtx,
313319
ExcludedContainerTypes))
314320
return;
315-
if (isVariableUnused(Var, BlockStmt, Context)) {
316-
auto Diagnostic =
317-
diag(Var.getLocation(),
318-
"the %select{|const qualified }0variable %1 is copy-constructed "
319-
"from a const reference but is never used; consider "
320-
"removing the statement")
321-
<< IsConstQualified << &Var;
322-
if (IssueFix)
323-
recordRemoval(Stmt, Context, Diagnostic);
324-
} else {
325-
auto Diagnostic =
326-
diag(Var.getLocation(),
327-
"the %select{|const qualified }0variable %1 is copy-constructed "
328-
"from a const reference%select{ but is only used as const "
329-
"reference|}0; consider making it a const reference")
330-
<< IsConstQualified << &Var;
331-
if (IssueFix)
332-
recordFixes(Var, Context, Diagnostic);
333-
}
321+
diagnoseCopyFromMethodReturn(Ctx);
334322
}
335323

336324
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
337-
const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
338-
const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
339-
if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
340-
!isInitializingVariableImmutable(OldVar, BlockStmt, Context,
325+
const CheckContext &Ctx, const VarDecl &OldVar) {
326+
if (!Ctx.IsVarOnlyUsedAsConst ||
327+
!isInitializingVariableImmutable(OldVar, Ctx.BlockStmt, Ctx.ASTCtx,
341328
ExcludedContainerTypes))
342329
return;
330+
diagnoseCopyFromLocalVar(Ctx, OldVar);
331+
}
343332

344-
if (isVariableUnused(NewVar, BlockStmt, Context)) {
345-
auto Diagnostic = diag(NewVar.getLocation(),
346-
"local copy %0 of the variable %1 is never modified "
347-
"and never used; "
348-
"consider removing the statement")
349-
<< &NewVar << &OldVar;
350-
if (IssueFix)
351-
recordRemoval(Stmt, Context, Diagnostic);
352-
} else {
353-
auto Diagnostic =
354-
diag(NewVar.getLocation(),
355-
"local copy %0 of the variable %1 is never modified; "
356-
"consider avoiding the copy")
357-
<< &NewVar << &OldVar;
358-
if (IssueFix)
359-
recordFixes(NewVar, Context, Diagnostic);
333+
void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
334+
const CheckContext &Ctx) {
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+
346+
void UnnecessaryCopyInitialization::diagnoseCopyFromLocalVar(
347+
const CheckContext &Ctx, const VarDecl &OldVar) {
348+
auto Diagnostic =
349+
diag(Ctx.Var.getLocation(),
350+
"local copy %1 of the variable %0 is never modified%select{"
351+
"| and never used}2; consider %select{avoiding the copy|removing "
352+
"the statement}2")
353+
<< &OldVar << &Ctx.Var << Ctx.IsVarUnused;
354+
maybeIssueFixes(Ctx, Diagnostic);
355+
}
356+
357+
void UnnecessaryCopyInitialization::maybeIssueFixes(
358+
const CheckContext &Ctx, DiagnosticBuilder &Diagnostic) {
359+
if (Ctx.IssueFix) {
360+
if (Ctx.IsVarUnused)
361+
recordRemoval(Ctx.VarDeclStmt, Ctx.ASTCtx, Diagnostic);
362+
else
363+
recordFixes(Ctx.Var, Ctx.ASTCtx, Diagnostic);
360364
}
361365
}
362366

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,32 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
3232
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
3333
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
3434

35+
protected:
36+
// A helper to manipulate the state common to
37+
// `CopyFromMethodReturn` and `CopyFromLocalVar`.
38+
struct CheckContext {
39+
const VarDecl &Var;
40+
const Stmt &BlockStmt;
41+
const DeclStmt &VarDeclStmt;
42+
clang::ASTContext &ASTCtx;
43+
const bool IssueFix;
44+
const bool IsVarUnused;
45+
const bool IsVarOnlyUsedAsConst;
46+
};
47+
48+
// Create diagnostics. These are virtual so that derived classes can change
49+
// behaviour.
50+
virtual void diagnoseCopyFromMethodReturn(const CheckContext &Ctx);
51+
virtual void diagnoseCopyFromLocalVar(const CheckContext &Ctx,
52+
const VarDecl &OldVar);
53+
3554
private:
36-
void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
37-
const DeclStmt &Stmt, bool IssueFix,
38-
const VarDecl *ObjectArg,
39-
ASTContext &Context);
40-
void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
41-
const Stmt &BlockStmt, const DeclStmt &Stmt,
42-
bool IssueFix, ASTContext &Context);
55+
void handleCopyFromMethodReturn(const CheckContext &Ctx,
56+
const VarDecl *ObjectArg);
57+
void handleCopyFromLocalVar(const CheckContext &Ctx, const VarDecl &OldVar);
58+
59+
void maybeIssueFixes(const CheckContext &Ctx, DiagnosticBuilder &Diagnostic);
60+
4361
const std::vector<StringRef> AllowedTypes;
4462
const std::vector<StringRef> ExcludedContainerTypes;
4563
};

0 commit comments

Comments
 (0)