[clang-tidy] Use ExprMutationAnalyzer in performance-unnecessary-value-param

[shuaiwang]

Summary

This yields better recall as ExprMutationAnalyzer is more accurate.

One common pattern this check is now able to catch is:

void foo(std::vector<X> v) {
  for (const auto& elm : v) {
    // ...
  }
}

[lebedev.ri]

It is great when the test cases are included :)

[shuaiwang]

Add test case

[hokein]

Mostly good. A few nits.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

103

This comment needs to be updated.

108

Is this a new fix or a special case not being caught by ExprMutationAnalyzer? Do we have a test case covered it?

[shuaiwang]

Update comments.

[shuaiwang]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

108

It's a special case.
ExprMutationAnalyzer is designed to analyze within a "scope" indicated by a Stmt. So when trying to analyze a "function", merely looking at function body is not sufficient because CXXCtorInitializer is not part of the function body. So far we only care about this special case when trying to figure out whether a function param is mutated or not, which is exactly what this check is doing. We might consider making this part of ExprMutationAnalyzer but IMO that depends on whether there are more use cases of this special case in the future because we'll need some tricks there to match different types of top level nodes.

This is already captured in existing unit test case, which basically looks like this:

class Foo {
public:
  Foo(ExpensiveObj x) : x(std::move(x)) {}
private:
  ExpensiveObj x;
};

[hokein]

LGTM.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

108

Thanks for the detailed clarification! That makes sense. Could you also add a proper comment in the code?

[shuaiwang]

Add comments explaining CXXCtorInitializer check

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

 //===--- UnnecessaryValueParamCheck.cpp - clang-tidy-----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "UnnecessaryValueParamCheck.h"
 
 #include "../utils/DeclRefExprUtils.h"
+#include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/Matchers.h"
 #include "../utils/TypeTraits.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 
 using namespace clang::ast_matchers;
Finder->addMatcher(
                    has(typeLoc(forEach(ExpensiveValueParamDecl))),
                    unless(isInstantiated()), decl().bind("functionDecl")),
       this);
 }
 
 void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
   const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
-  const size_t Index = std::find(Function->parameters().begin(),
-                                 Function->parameters().end(), Param) -
-                       Function->parameters().begin();
-  bool IsConstQualified =
-      Param->getType().getCanonicalType().isConstQualified();
-
-  auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
-      *Param, *Function, *Result.Context);
-  auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
-      *Param, *Function, *Result.Context);
 
   // Do not trigger on non-const value parameters when they are not only used as
   // const.
-  if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
+  if (utils::ExprMutationAnalyzer(Function->getBody(), Result.Context)
+          .isMutated(Param))
     return;
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Function)) {
+    for (const auto *Init : Ctor->inits()) {
+      if (utils::ExprMutationAnalyzer(Init->getInit(), Result.Context)
+              .isMutated(Param))
+        return;
+    }
+  }
+
+  const bool IsConstQualified =
+      Param->getType().getCanonicalType().isConstQualified();
 
   // If the parameter is non-const, check if it has a move constructor and is
   // only referenced once to copy-construct another object or whether it has a
   // move assignment operator and is only referenced once when copy-assigned.
   // In this case wrap DeclRefExpr with std::move() to avoid the unnecessary
   // copy.
-  if (!IsConstQualified && AllDeclRefExprs.size() == 1) {
+  if (!IsConstQualified) {
+    auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
+        *Param, *Function, *Result.Context);
+    if (AllDeclRefExprs.size() == 1) {
       auto CanonicalType = Param->getType().getCanonicalType();
-    const auto &DeclRefExpr  = **AllDeclRefExprs.begin();
+      const auto &DeclRefExpr = **AllDeclRefExprs.begin();
 
       if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
           ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
             utils::decl_ref_expr::isCopyConstructorArgument(
                 DeclRefExpr, *Function, *Result.Context)) ||
            (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
             utils::decl_ref_expr::isCopyAssignmentArgument(
                 DeclRefExpr, *Function, *Result.Context)))) {
         handleMoveFix(*Param, DeclRefExpr, *Result.Context);
         return;
       }
     }
+  }
+
+  const size_t Index = std::find(Function->parameters().begin(),
+                                 Function->parameters().end(), Param) -
+                       Function->parameters().begin();
 
   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; "