[clang-tools-extra] [clang-tidy][performance-unnecessary-value-param] Make `handleMoveFix` virtual (PR #99867)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tidy

Author: Clement Courbet (legrosbuffle)

<details>
<summary>Changes</summary>

... so that downstream checks can override behaviour to do additional processing.

Refactor the rest of the logic to `handleConstRefFix` (which is also `virtual`).

This is otherwise and NFC.

This is similar to https://github.com/llvm/llvm-project/pull/73921 but for `performance-unnecessary-value-param`.

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


2 Files Affected:

- (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+38-31) 
- (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h (+9-3) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index 5a4c2363bd8af..3a255c5c133f1 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -119,65 +119,72 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
     }
   }
 
-  const size_t Index = llvm::find(Function->parameters(), Param) -
-                       Function->parameters().begin();
+  handleConstRefFix(*Function, *Param, *Result.Context);
+}
+
+void UnnecessaryValueParamCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  Inserter.registerPreprocessor(PP);
+}
+
+void UnnecessaryValueParamCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle", Inserter.getStyle());
+  Options.store(Opts, "AllowedTypes",
+                utils::options::serializeStringList(AllowedTypes));
+}
+
+void UnnecessaryValueParamCheck::onEndOfTranslationUnit() {
+  MutationAnalyzerCache.clear();
+}
+
+void UnnecessaryValueParamCheck::handleConstRefFix(const FunctionDecl &Function,
+                                                   const ParmVarDecl &Param,
+                                                   ASTContext &Context) {
+  const size_t Index =
+      llvm::find(Function.parameters(), &Param) - Function.parameters().begin();
+  const bool IsConstQualified =
+      Param.getType().getCanonicalType().isConstQualified();
 
   auto Diag =
-      diag(Param->getLocation(),
+      diag(Param.getLocation(),
            "the %select{|const qualified }0parameter %1 is copied for each "
            "invocation%select{ but only used as a const reference|}0; consider "
            "making it a %select{const |}0reference")
-      << IsConstQualified << paramNameOrIndex(Param->getName(), Index);
+      << IsConstQualified << paramNameOrIndex(Param.getName(), Index);
   // Do not propose fixes when:
   // 1. the ParmVarDecl is in a macro, since we cannot place them correctly
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //    compilation unit as the signature change could introduce build errors.
   // 4. the function is an explicit template/ specialization.
-  const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Function);
-  if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
-      isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
-      Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+  const auto *Method = llvm::dyn_cast<CXXMethodDecl>(&Function);
+  if (Param.getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
+      isReferencedOutsideOfCallExpr(Function, Context) ||
+      Function.getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
     return;
-  for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
+  for (const auto *FunctionDecl = &Function; FunctionDecl != nullptr;
        FunctionDecl = FunctionDecl->getPreviousDecl()) {
     const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
-    Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
-                                                   *Result.Context);
+    Diag << utils::fixit::changeVarDeclToReference(CurrentParam, Context);
     // The parameter of each declaration needs to be checked individually as to
     // whether it is const or not as constness can differ between definition and
     // declaration.
     if (!CurrentParam.getType().getCanonicalType().isConstQualified()) {
       if (std::optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
-              CurrentParam, *Result.Context, DeclSpec::TQ::TQ_const))
+              CurrentParam, Context, DeclSpec::TQ::TQ_const))
         Diag << *Fix;
     }
   }
 }
 
-void UnnecessaryValueParamCheck::registerPPCallbacks(
-    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
-  Inserter.registerPreprocessor(PP);
-}
-
-void UnnecessaryValueParamCheck::storeOptions(
-    ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "IncludeStyle", Inserter.getStyle());
-  Options.store(Opts, "AllowedTypes",
-                utils::options::serializeStringList(AllowedTypes));
-}
-
-void UnnecessaryValueParamCheck::onEndOfTranslationUnit() {
-  MutationAnalyzerCache.clear();
-}
-
-void UnnecessaryValueParamCheck::handleMoveFix(const ParmVarDecl &Var,
+void UnnecessaryValueParamCheck::handleMoveFix(const ParmVarDecl &Param,
                                                const DeclRefExpr &CopyArgument,
-                                               const ASTContext &Context) {
+                                               ASTContext &Context) {
   auto Diag = diag(CopyArgument.getBeginLoc(),
                    "parameter %0 is passed by value and only copied once; "
                    "consider moving it to avoid unnecessary copies")
-              << &Var;
+              << &Param;
   // Do not propose fixes in macros since we cannot place them correctly.
   if (CopyArgument.getBeginLoc().isMacroID())
     return;
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
index 7250bffd20b2f..8bfd814d16357 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
@@ -33,10 +33,16 @@ class UnnecessaryValueParamCheck : public ClangTidyCheck {
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void onEndOfTranslationUnit() override;
 
-private:
-  void handleMoveFix(const ParmVarDecl &Var, const DeclRefExpr &CopyArgument,
-                     const ASTContext &Context);
+protected:
+  // Create diagnostics. These are virtual so that derived classes can change
+  // behaviour.
+  virtual void handleMoveFix(const ParmVarDecl &Param,
+                             const DeclRefExpr &CopyArgument,
+                             ASTContext &Context);
+  virtual void handleConstRefFix(const FunctionDecl &Function,
+                                 const ParmVarDecl &Param, ASTContext &Context);
 
+private:
   ExprMutationAnalyzer::Memoized MutationAnalyzerCache;
   utils::IncludeInserter Inserter;
   const std::vector<StringRef> AllowedTypes;

``````````

</details>


https://github.com/llvm/llvm-project/pull/99867