[clang-tools-extra] 04ce9a3 - [clang-tidy] performance-unnecessary-copy-init: Add a hook... (#73921)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 6 23:31:20 PST 2023
Author: Clement Courbet
Date: 2023-12-07T08:31:16+01:00
New Revision: 04ce9a34ea82647a61b4e2a2a3cc5c93cc2f0d7d
URL: https://github.com/llvm/llvm-project/commit/04ce9a34ea82647a61b4e2a2a3cc5c93cc2f0d7d
DIFF: https://github.com/llvm/llvm-project/commit/04ce9a34ea82647a61b4e2a2a3cc5c93cc2f0d7d.diff
LOG: [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.
Added:
Modified:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfc..dfe12c5b6007d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
#include "clang/AST/Decl.h"
#include "clang/Basic/Diagnostic.h"
#include <optional>
+#include <utility>
namespace clang::tidy::performance {
namespace {
@@ -263,19 +264,25 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
void UnnecessaryCopyInitialization::check(
const MatchFinder::MatchResult &Result) {
- const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
+ const auto &NewVar = *Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
+ const auto &BlockStmt = *Result.Nodes.getNodeAs<Stmt>("blockStmt");
+ const auto &VarDeclStmt = *Result.Nodes.getNodeAs<DeclStmt>("declStmt");
+ // Do not propose fixes if the DeclStmt has multiple VarDecls or in
+ // macros since we cannot place them correctly.
+ const bool IssueFix =
+ VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
+ const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, *Result.Context);
+ const bool IsVarOnlyUsedAsConst =
+ isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
+ const CheckContext Context{
+ NewVar, BlockStmt, VarDeclStmt, *Result.Context,
+ IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>(OldVarDeclId);
const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>(ObjectArgId);
- const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt");
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
- const auto *Stmt = Result.Nodes.getNodeAs<DeclStmt>("declStmt");
TraversalKindScope RAII(*Result.Context, TK_AsIs);
- // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
- // since we cannot place them correctly.
- bool IssueFix = Stmt->isSingleDecl() && !NewVar->getLocation().isMacroID();
-
// A constructor that looks like T(const T& t, bool arg = false) counts as a
// copy only when it is called with default arguments for the arguments after
// the first.
@@ -289,74 +296,71 @@ void UnnecessaryCopyInitialization::check(
// instantiations where the types
diff er and rely on implicit conversion would
// no longer compile if we switched to a reference.
if (
diff erentReplacedTemplateParams(
- NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes),
+ Context.Var.getType(), constructorArgumentType(OldVar, Result.Nodes),
*Result.Context))
return;
if (OldVar == nullptr) {
- handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
- *Result.Context);
+ // `auto NewVar = functionCall();`
+ handleCopyFromMethodReturn(Context, ObjectArg);
} else {
- handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
- *Result.Context);
+ // `auto NewVar = OldVar;`
+ handleCopyFromLocalVar(Context, *OldVar);
}
}
void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
- const VarDecl &Var, const Stmt &BlockStmt, const DeclStmt &Stmt,
- bool IssueFix, const VarDecl *ObjectArg, ASTContext &Context) {
- bool IsConstQualified = Var.getType().isConstQualified();
- if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
+ const CheckContext &Ctx, const VarDecl *ObjectArg) {
+ bool IsConstQualified = Ctx.Var.getType().isConstQualified();
+ if (!IsConstQualified && !Ctx.IsVarOnlyUsedAsConst)
return;
if (ObjectArg != nullptr &&
- !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
+ !isInitializingVariableImmutable(*ObjectArg, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
return;
- if (isVariableUnused(Var, BlockStmt, Context)) {
- auto Diagnostic =
- diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
- << IsConstQualified << &Var;
- if (IssueFix)
- recordRemoval(Stmt, Context, Diagnostic);
- } else {
- auto Diagnostic =
- diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
- << IsConstQualified << &Var;
- if (IssueFix)
- recordFixes(Var, Context, Diagnostic);
- }
+ diagnoseCopyFromMethodReturn(Ctx);
}
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
- const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
- const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
- if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
- !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
+ const CheckContext &Ctx, const VarDecl &OldVar) {
+ if (!Ctx.IsVarOnlyUsedAsConst ||
+ !isInitializingVariableImmutable(OldVar, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
return;
+ diagnoseCopyFromLocalVar(Ctx, OldVar);
+}
- if (isVariableUnused(NewVar, BlockStmt, Context)) {
- auto Diagnostic = diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified "
- "and never used; "
- "consider removing the statement")
- << &NewVar << &OldVar;
- if (IssueFix)
- recordRemoval(Stmt, Context, Diagnostic);
- } else {
- auto Diagnostic =
- diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
- << &NewVar << &OldVar;
- if (IssueFix)
- recordFixes(NewVar, Context, Diagnostic);
+void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
+ const CheckContext &Ctx) {
+ auto Diagnostic =
+ diag(Ctx.Var.getLocation(),
+ "the %select{|const qualified }0variable %1 is "
+ "copy-constructed "
+ "from a const reference%select{%select{ but is only used as const "
+ "reference|}0| but is never used}2; consider "
+ "%select{making it a const reference|removing the statement}2")
+ << Ctx.Var.getType().isConstQualified() << &Ctx.Var << Ctx.IsVarUnused;
+ maybeIssueFixes(Ctx, Diagnostic);
+}
+
+void UnnecessaryCopyInitialization::diagnoseCopyFromLocalVar(
+ const CheckContext &Ctx, const VarDecl &OldVar) {
+ auto Diagnostic =
+ diag(Ctx.Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified%select{"
+ "| and never used}2; consider %select{avoiding the copy|removing "
+ "the statement}2")
+ << &OldVar << &Ctx.Var << Ctx.IsVarUnused;
+ maybeIssueFixes(Ctx, Diagnostic);
+}
+
+void UnnecessaryCopyInitialization::maybeIssueFixes(
+ const CheckContext &Ctx, DiagnosticBuilder &Diagnostic) {
+ if (Ctx.IssueFix) {
+ if (Ctx.IsVarUnused)
+ recordRemoval(Ctx.VarDeclStmt, Ctx.ASTCtx, Diagnostic);
+ else
+ recordFixes(Ctx.Var, Ctx.ASTCtx, Diagnostic);
}
}
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index ea009ba9979de..ab0f1ecf61063 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -32,14 +32,32 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+protected:
+ // A helper to manipulate the state common to
+ // `CopyFromMethodReturn` and `CopyFromLocalVar`.
+ struct CheckContext {
+ const VarDecl &Var;
+ const Stmt &BlockStmt;
+ const DeclStmt &VarDeclStmt;
+ clang::ASTContext &ASTCtx;
+ const bool IssueFix;
+ const bool IsVarUnused;
+ const bool IsVarOnlyUsedAsConst;
+ };
+
+ // Create diagnostics. These are virtual so that derived classes can change
+ // behaviour.
+ virtual void diagnoseCopyFromMethodReturn(const CheckContext &Ctx);
+ virtual void diagnoseCopyFromLocalVar(const CheckContext &Ctx,
+ const VarDecl &OldVar);
+
private:
- void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
- const DeclStmt &Stmt, bool IssueFix,
- const VarDecl *ObjectArg,
- ASTContext &Context);
- void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
- const Stmt &BlockStmt, const DeclStmt &Stmt,
- bool IssueFix, ASTContext &Context);
+ void handleCopyFromMethodReturn(const CheckContext &Ctx,
+ const VarDecl *ObjectArg);
+ void handleCopyFromLocalVar(const CheckContext &Ctx, const VarDecl &OldVar);
+
+ void maybeIssueFixes(const CheckContext &Ctx, DiagnosticBuilder &Diagnostic);
+
const std::vector<StringRef> AllowedTypes;
const std::vector<StringRef> ExcludedContainerTypes;
};
More information about the cfe-commits
mailing list