[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:44:29 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 a37b76a3ad04074eadf4ccb781c6f5e14bcea7eb 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           |  35 ++++--
 2 files changed, 85 insertions(+), 63 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..da48a3c4e2b2f98 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,19 +33,32 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
   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);
+  // A helper to manipulate the state common to
+  // `CopyFromMethodReturn` and `CopyFromLocalVar`.
+  struct CheckContext {
+    CheckContext(const ast_matchers::MatchFinder::MatchResult &Result);
+    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,
+                                            const VarDecl *ObjectArg);
+  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