[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 ↗

(On Diff #158408)

This comment needs to be updated.

108 ↗

(On Diff #158408)

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 ↗

(On Diff #158408)

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 ↗

(On Diff #158408)

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-tools-extra/trunk/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;
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 namespace {
 
 std::string paramNameOrIndex(StringRef Name, size_t Index) {
   return (Name.empty() ? llvm::Twine('#') + llvm::Twine(Index + 1)
                        : llvm::Twine('\'') + Name + llvm::Twine('\''))
       .str();
 }
 
-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();
 }
 
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))
+  // Do not trigger on non-const value parameters when they are mutated either
+  // within the function body or within init expression(s) when the function is
+  // a ctor.
+  if (utils::ExprMutationAnalyzer(Function->getBody(), Result.Context)
+          .isMutated(Param))
     return;
+  // CXXCtorInitializer might also mutate Param but they're not part of function
+  // body, so check them separately here.
+  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; "

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

struct ExpensiveToCopyType {
   virtual ~ExpensiveToCopyType();
 };
 
 void mutate(ExpensiveToCopyType &);
 void mutate(ExpensiveToCopyType *);
 void useAsConstReference(const ExpensiveToCopyType &);
 void useByValue(ExpensiveToCopyType);
 
+template <class T> class Vector {
+ public:
+  using iterator = T*;
+  using const_iterator = const T*;
+
+  Vector(const Vector&);
+  Vector& operator=(const Vector&);
+
+  iterator begin();
+  iterator end();
+  const_iterator begin() const;
+  const_iterator end() const;
+};
+
 // This class simulates std::pair<>. It is trivially copy constructible
 // and trivially destructible, but not trivially copy assignable.
 class SomewhatTrivial {
  public:
   SomewhatTrivial();
   SomewhatTrivial(const SomewhatTrivial&) = default;
   ~SomewhatTrivial() = default;
   SomewhatTrivial& operator=(const SomewhatTrivial&);
void positiveExpensiveValue(ExpensiveToCopyType Obj) {
   // CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'Obj' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
   // CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj) {
   Obj.constReference();
   useAsConstReference(Obj);
   auto Copy = Obj;
   useByValue(Obj);
 }
 
+void positiveVector(Vector<ExpensiveToCopyType> V) {
+  // CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'V' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
+  // CHECK-FIXES: void positiveVector(const Vector<ExpensiveToCopyType>& V) {
+  for (const auto& Obj : V) {
+    useByValue(Obj);
+  }
+}
+
 void positiveWithComment(const ExpensiveToCopyType /* important */ S);
 // CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S);
 void positiveWithComment(const ExpensiveToCopyType /* important */ S) {
   // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the const qualified
   // CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S) {
 }
 
 void positiveUnnamedParam(const ExpensiveToCopyType) {