[clang-tidy] performance-unnecessary-copy-init: Add a hook...

[legrosbuffle]

... so that derived checks can derived checks can observe for which variables a warning has been emitted. Does nothing by default, which makes this an NFC.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Clement Courbet (legrosbuffle)

Changes

... so that derived checks can implement custom behaviour. Does nothing by default, which makes this an NFC.

---

Full diff: https://github.com/llvm/llvm-project/pull/73921.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp (+15-9)
• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h (+7-2)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..966071e92833b87 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -294,24 +294,28 @@ void UnnecessaryCopyInitialization::check(
     return;
 
   if (OldVar == nullptr) {
-    handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-                               *Result.Context);
+    if (handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+                                   ObjectArg, *Result.Context)) {
+      onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+    }
   } else {
-    handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-                           *Result.Context);
+    if (handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
+                               *Result.Context)) {
+      onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+    }  
   }
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
+bool 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))
-    return;
+    return false;
   if (ObjectArg != nullptr &&
       !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
                                        ExcludedContainerTypes))
-    return;
+    return false;
   if (isVariableUnused(Var, BlockStmt, Context)) {
     auto Diagnostic =
         diag(Var.getLocation(),
@@ -331,15 +335,16 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
     if (IssueFix)
       recordFixes(Var, Context, Diagnostic);
   }
+  return true;
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
+bool 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,
                                        ExcludedContainerTypes))
-    return;
+    return false;
 
   if (isVariableUnused(NewVar, BlockStmt, Context)) {
     auto Diagnostic = diag(NewVar.getLocation(),
@@ -358,6 +363,7 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
     if (IssueFix)
       recordFixes(NewVar, Context, Diagnostic);
   }
+  return true;
 }
 
 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..7d76c9e7cab940c 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,13 +33,18 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
-  void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt,
+  bool 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,
+  bool handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar,
                               const Stmt &BlockStmt, const DeclStmt &Stmt,
                               bool IssueFix, ASTContext &Context);
+
+  // A hook called for each emitted warning.
+  virtual void onWarningEmitted(const VarDecl &Var, const Stmt &BlockStmt,
+                                ASTContext &context) {}
+
   const std::vector<StringRef> AllowedTypes;
   const std::vector<StringRef> ExcludedContainerTypes;
 };

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[PiotrZSL]

First what's a purpose of this change. Is there any other check that will need it or not ?
Second is that actually onWarningEmitted should be more a emitWarning, and those diag should be inside it, so that they could be changed if needed. Or condition required to emit a warning should be in separate methods.
With current implementation I don't see any "custom behaviour" that could be implemented, as this act more like observer.

[legrosbuffle]

First what's a purpose of this change. Is there any other check that will need it or not ?

We have a downstream check that wants to do additional processing after this one. We can't submit that check because it depends on a large piece of downstream infrastructure.

With current implementation I don't see any "custom behaviour" that could be implemented, as this act more like observer.

Indeed, the downstream check does not change the behaviour, it does extra stuff in addition what the check already does. I've rephrased the commit message to make that clearer.

[PiotrZSL]

In theory changes that are required for "private" checks should be done by you on some own private fork instead of adding that "unused" code here. But only option I see is simply moving diagnostic to separate methods, and make those methods virtual. That could be +- acceptable. Or moving "condition" into separate method. Adding empty methods & ifs to call that method is not an option.

[legrosbuffle]

In theory changes that are required for "private" checks should be done by you on some own private fork instead of adding that "unused" code here. But only option I see is simply moving diagnostic to separate methods, and make those methods virtual.

I see, we don't want code that's dead upstream. Makes sense. I'll do something like what you're suggesting.

[legrosbuffle]

This is ready for another look. I took this as an opportunity to refactor the duplicated parts of the code. Let me know what you think.

[fberger]

LGTM

[PiotrZSL]

Last nits, and it will look fine.

[PiotrZSL]

LGTM

[legrosbuffle]

Thanks