[clang-tidy] UnnecessaryValueParamCheck - only warn for virtual methods

[flx]

Summary

As changing virtual methods could break method overrides disable applying the fix and just warn.

[alexfh]

LG after addressing the comment. Thanks!

test/clang-tidy/performance-unnecessary-value-param.cpp

234 ↗

(On Diff #62535)

Please add a test for an override not explicitly marked "virtual" or "override".

[flx]

test/clang-tidy/performance-unnecessary-value-param.cpp

234 ↗

(On Diff #62535)

That case is already covered in line 180. I moved these tests up now to have them grouped together.

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

auto Diag =
       diag(Param->getLocation(),
            IsConstQualified ? "the const qualified parameter %0 is "
                               "copied for each invocation; consider "
                               "making it a reference"
                             : "the parameter %0 is copied for each "
                               "invocation but only used as a const reference; "
                               "consider making it a const reference")
       << paramNameOrIndex(Param->getName(), Index);
-  // Do not propose fixes in macros since we cannot place them correctly.
-  if (Param->getLocStart().isMacroID())
+  // Do not propose fixes in macros since we cannot place them correctly, or if
+  // function is virtual as it might break overrides.
+  const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Function);
+  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()))
     return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
        FunctionDecl = FunctionDecl->getPreviousDecl()) {
     const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
     Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
                                                            *Result.Context);
     if (!IsConstQualified)
       Diag << utils::fixit::changeVarDeclToConst(CurrentParam);

clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp

 };
 
 struct NegativeOverriddenMethod : public VirtualMethod {
   void handle(ExpensiveToCopyType Overridden) const {
     // CHECK-FIXES: handle(ExpensiveToCopyType Overridden) const {
   }
 };
 
+struct VirtualMethodWarningOnly {
+  virtual void methodWithExpensiveValueParam(ExpensiveToCopyType T) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'T' is copied
+  // CHECK-FIXES: virtual void methodWithExpensiveValueParam(ExpensiveToCopyType T) {}
+  virtual ~VirtualMethodWarningOnly() {}
+};
+
+struct PositiveNonVirualMethod {
+  void method(const ExpensiveToCopyType T) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:41: warning: the const qualified parameter 'T' is copied
+  // CHECK-FIXES: void method(const ExpensiveToCopyType& T) {}
+};
+
 struct NegativeDeletedMethod {
   ~NegativeDeletedMethod() {}
   NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
   // CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
 };
 
 void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
   M.constMethod();