[ClangTidy - performance-unnecessary-value-param]: Do not issue fix for functions that are referenced outside of callExpr

[flx]

Summary

Suppress fixes for functions that are referenced within the compilation unit outside of a call expression as the signature change could break the code referencing the function.

We still issue a warning in this case so that users can decide to manually change the function signature.

[aaron.ballman]

Aside from a commenting nit, LGTM.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

130

Not grammatically correct; should be "1. when the ParmVarDecl is in a macro, since..."

[flx]

Thanks for the review!

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

 template <typename S>
 bool isSubset(const S &SubsetCandidate, const S &SupersetCandidate) {
   for (const auto &E : SubsetCandidate)
     if (SupersetCandidate.count(E) == 0)
       return false;
   return true;
 }
 
+bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function,
+                                   ASTContext &Context) {
+  auto Matches = match(declRefExpr(to(functionDecl(equalsNode(&Function))),
+                                   unless(hasAncestor(callExpr()))),
+                       Context);
+  return !Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
           Options.get("IncludeStyle", "llvm"))) {}
 
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, or if
-  // function is virtual as it might break overrides.
+  // 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.
   const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Function);
-  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()))
+  if (Param->getLocStart().isMacroID() || (Method && Method->isVirtual()) ||
+      isReferencedOutsideOfCallExpr(*Function, *Result.Context))
     return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
        FunctionDecl = FunctionDecl->getPreviousDecl()) {
     const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
     Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
                                                            *Result.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

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

void PositiveConstDeclaration(ExpensiveToCopyType A) {
   // CHECK-FIXES: void PositiveConstDeclaration(const ExpensiveToCopyType& A) {
 }
 
 void PositiveNonConstDeclaration(ExpensiveToCopyType A);
 // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A);
 void PositiveNonConstDeclaration(const ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'A'
   // CHECK-FIXES: void PositiveNonConstDeclaration(const ExpensiveToCopyType& A) {
+
+void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:75: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveOnlyMessageAsReferencedInCompilationUnit(ExpensiveToCopyType A) {
+}
+
+void ReferenceFunctionOutsideOfCallExpr() {
+  void (*ptr)(ExpensiveToCopyType) = &PositiveOnlyMessageAsReferencedInCompilationUnit;
+}
+
+void PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType A) {
+  // CHECK-MESSAGES: [[@LINE-1]]:66: warning: the parameter 'A' is copied
+  // CHECK-FIXES: void PositiveMessageAndFixAsFunctionIsCalled(const ExpensiveToCopyType& A) {
+}
+
+void ReferenceFunctionByCallingIt() {
+  PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
 }