[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)
Clement Courbet via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 1 01:43:07 PST 2023
https://github.com/legrosbuffle updated https://github.com/llvm/llvm-project/pull/73921
>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH 1/2] [clang-tidy] performance-unnecessary-copy-init
Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
.../UnnecessaryCopyInitialization.cpp | 73 +++++++++----------
.../UnnecessaryCopyInitialization.h | 7 ++
2 files changed, 41 insertions(+), 39 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..a9ef3faf8c343c9 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 {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
}
}
+void UnnecessaryCopyInitialization::makeDiagnostic(
+ DiagnosticBuilder Diagnostic, const VarDecl &Var, const Stmt &BlockStmt,
+ const DeclStmt &Stmt, ASTContext &Context, bool IssueFix) {
+ const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+ Diagnostic << &Var << IsVarUnused;
+ if (!IssueFix)
+ return;
+ if (IsVarUnused)
+ recordRemoval(Stmt, Context, Diagnostic);
+ else
+ recordFixes(Var, Context, Diagnostic);
+}
+
void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
const VarDecl &Var, const Stmt &BlockStmt, const DeclStmt &Stmt,
bool IssueFix, const VarDecl *ObjectArg, ASTContext &Context) {
@@ -312,52 +326,33 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
!isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
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);
- }
+
+ auto Diagnostic =
+ diag(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")
+ << IsConstQualified;
+ makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
}
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
- const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
+ const VarDecl &Var, const VarDecl &OldVar, const Stmt &BlockStmt,
const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
- if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+ if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
!isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
return;
-
- 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);
- }
+ auto Diagnostic = diag(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;
+ makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
}
void UnnecessaryCopyInitialization::storeOptions(
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index ea009ba9979de97..0eadca8c0137c45 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -32,6 +32,12 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+protected:
+ // This is virtual so that derived classes can implement additional behavior.
+ virtual void makeDiagnostic(DiagnosticBuilder Diagnostic, const VarDecl &Var,
+ const Stmt &BlockStmt, const DeclStmt &Stmt,
+ ASTContext &Context, bool IssueFix);
+
private:
void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
const DeclStmt &Stmt, bool IssueFix,
@@ -40,6 +46,7 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
const Stmt &BlockStmt, const DeclStmt &Stmt,
bool IssueFix, ASTContext &Context);
+
const std::vector<StringRef> AllowedTypes;
const std::vector<StringRef> ExcludedContainerTypes;
};
>From 824be23efb6d249b417b2d054efc09ec19df667f Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Fri, 1 Dec 2023 10:41:48 +0100
Subject: [PATCH 2/2] fixup! [clang-tidy] performance-unnecessary-copy-init
---
.../UnnecessaryCopyInitialization.cpp | 113 ++++++++++--------
.../UnnecessaryCopyInitialization.h | 89 +++++++-------
2 files changed, 104 insertions(+), 98 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index a9ef3faf8c343c9..16dff8cdd882177 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -262,21 +262,27 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
this);
}
+UnnecessaryCopyInitialization::CheckContext::CheckContext(
+ const ast_matchers::MatchFinder::MatchResult &Result)
+ : Var(*Result.Nodes.getNodeAs<VarDecl>("newVarDecl")),
+ BlockStmt(*Result.Nodes.getNodeAs<Stmt>("blockStmt")),
+ VarDeclStmt(*Result.Nodes.getNodeAs<DeclStmt>("declStmt")),
+ ASTCtx(*Result.Context),
+ // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros
+ // since we cannot place them correctly.
+ IssueFix(VarDeclStmt.isSingleDecl() && !Var.getLocation().isMacroID()),
+ IsVarUnused(isVariableUnused(Var, BlockStmt, ASTCtx)),
+ IsVarOnlyUsedAsConst(isOnlyUsedAsConst(Var, BlockStmt, ASTCtx)) {}
+
void UnnecessaryCopyInitialization::check(
const MatchFinder::MatchResult &Result) {
- const auto *NewVar = Result.Nodes.getNodeAs<VarDecl>("newVarDecl");
+ const CheckContext Context(Result);
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.
@@ -290,69 +296,72 @@ void UnnecessaryCopyInitialization::check(
// instantiations where the types differ and rely on implicit conversion would
// no longer compile if we switched to a reference.
if (differentReplacedTemplateParams(
- 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::makeDiagnostic(
- DiagnosticBuilder Diagnostic, const VarDecl &Var, const Stmt &BlockStmt,
- const DeclStmt &Stmt, ASTContext &Context, bool IssueFix) {
- const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
- Diagnostic << &Var << IsVarUnused;
- if (!IssueFix)
- return;
- if (IsVarUnused)
- recordRemoval(Stmt, Context, Diagnostic);
- else
- recordFixes(Var, Context, Diagnostic);
-}
-
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;
-
- auto Diagnostic =
- diag(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")
- << IsConstQualified;
- makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
- IssueFix);
+ diagnoseCopyFromMethodReturn(Ctx, ObjectArg);
}
void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
- const VarDecl &Var, const VarDecl &OldVar, const Stmt &BlockStmt,
- const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
- if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
- !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
+ const CheckContext &Ctx, const VarDecl &OldVar) {
+ if (!Ctx.IsVarOnlyUsedAsConst ||
+ !isInitializingVariableImmutable(OldVar, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
return;
- auto Diagnostic = diag(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;
- makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
- IssueFix);
+ diagnoseCopyFromLocalVar(Ctx, OldVar);
+}
+
+void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
+ const CheckContext &Ctx, const VarDecl *ObjectArg) {
+ 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);
+ }
+ }
}
void UnnecessaryCopyInitialization::storeOptions(
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index 0eadca8c0137c45..a45aef4e73f1486 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -1,56 +1,53 @@
-//===--- UnnecessaryCopyInitialization.h - clang-tidy------------*- C++ -*-===//
+#ifndef DEVTOOLS_CYMBAL_CLANG_TIDY_CUSTOM_UNNECESSARY_COPY_INIT_H_
+#define DEVTOOLS_CYMBAL_CLANG_TIDY_CUSTOM_UNNECESSARY_COPY_INIT_H_
+
+#include "devtools/cymbal/clang_tidy/extra_info.h"
+#include "third_party/llvm/llvm-project/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "third_party/llvm/llvm-project/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h"
+#include "third_party/llvm/llvm-project/clang/include/clang/AST/Decl.h"
+#include "third_party/llvm/llvm-project/clang/include/clang/AST/Stmt.h"
+#include "third_party/llvm/llvm-project/clang/include/clang/Basic/Diagnostic.h"
+
+namespace clang {
+namespace tidy {
+namespace custom {
+
+// A wrapper for `performance-unnecessary-copy-init` that additionally emits
+// go/gwp-clang-tidy annotations.
//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
+// For the user-facing documentation see:
+// http://go/clang-tidy/checks/google3-custom-unnecessary-copy-init
+class UnnecessaryCopyInitCheck
+ : public performance::UnnecessaryCopyInitialization {
+public:
+ using UnnecessaryCopyInitialization::UnnecessaryCopyInitialization;
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
+ void onStartOfTranslationUnit() override {
+ parent_function_map_.StartTranslationUnit();
+ performance::UnnecessaryCopyInitialization::onStartOfTranslationUnit();
+ }
-#include "../ClangTidyCheck.h"
-#include "clang/AST/Decl.h"
+private:
+ using Base = performance::UnnecessaryCopyInitialization;
-namespace clang::tidy::performance {
+ void addAnnotations(const CheckContext &context);
-// The check detects local variable declarations that are copy initialized with
-// the const reference of a function call or the const reference of a method
-// call whose object is guaranteed to outlive the variable's scope and suggests
-// to use a const reference.
-//
-// The check currently only understands a subset of variables that are
-// guaranteed to outlive the const reference returned, namely: const variables,
-// const references, and const pointers to const.
-class UnnecessaryCopyInitialization : public ClangTidyCheck {
-public:
- UnnecessaryCopyInitialization(StringRef Name, ClangTidyContext *Context);
- bool isLanguageVersionSupported(const LangOptions &LangOpts) const override{
- return LangOpts.CPlusPlus;
+ void diagnoseCopyFromMethodReturn(const CheckContext &context,
+ const VarDecl *object_arg) override {
+ Base::diagnoseCopyFromMethodReturn(context, object_arg);
+ addAnnotations(context);
+ }
+ void diagnoseCopyFromLocalVar(const CheckContext &context,
+ const VarDecl &old_var) override {
+ Base::diagnoseCopyFromLocalVar(context, old_var);
+ addAnnotations(context);
}
- void registerMatchers(ast_matchers::MatchFinder *Finder) override;
- void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
- void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
-
-protected:
- // This is virtual so that derived classes can implement additional behavior.
- virtual void makeDiagnostic(DiagnosticBuilder Diagnostic, const VarDecl &Var,
- const Stmt &BlockStmt, const DeclStmt &Stmt,
- ASTContext &Context, bool IssueFix);
-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);
-
- const std::vector<StringRef> AllowedTypes;
- const std::vector<StringRef> ExcludedContainerTypes;
+ devtools::cymbal::ParentFunctionMap parent_function_map_;
};
-} // namespace clang::tidy::performance
+} // namespace custom
+} // namespace tidy
+} // namespace clang
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_INITIALIZATION_H
+#endif // DEVTOOLS_CYMBAL_CLANG_TIDY_CUSTOM_UNNECESSARY_COPY_INIT_H_
More information about the cfe-commits
mailing list