[clang-tidy] Handle constructors in performance-unnecessary-value-param

Summary:
modernize-pass-by-value doesn't warn about value parameters that
cannot be moved, so performance-unnecessary-value-param should.

Reviewers: aaron.ballman, flx, alexfh

Subscribers: JDevlieghere, cfe-commits

Differential Revision: https://reviews.llvm.org/D28022

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@290883 91177308-0d34-0410-b5e6-96231b3b80d8

1 files changed, 17 insertions, 28 deletions

diff --git a/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index edc3cf20..4c123181 100644
--- a/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -47,14 +47,14 @@ bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function,
   return !Matches.empty();
 }
 
-bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Decl &Decl,
                          ASTContext &Context) {
   auto Matches =
-      match(findAll(declRefExpr(
+      match(decl(forEachDescendant(declRefExpr(
                 equalsNode(&DeclRef),
                 unless(hasAncestor(stmt(anyOf(forStmt(), cxxForRangeStmt(),
-                                              whileStmt(), doStmt())))))),
-            Stmt, Context);
+                                              whileStmt(), doStmt()))))))),
+            Decl, Context);
   return Matches.empty();
 }
 
@@ -72,7 +72,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
                                                  unless(referenceType())))),
                   decl().bind("param"));
   Finder->addMatcher(
-      functionDecl(isDefinition(),
+      functionDecl(hasBody(stmt()), isDefinition(),
                    unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
                    unless(isInstantiated()),
                    has(typeLoc(forEach(ExpensiveValueParamDecl))),
@@ -89,22 +89,13 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   bool IsConstQualified =
       Param->getType().getCanonicalType().isConstQualified();
 
-  // Skip declarations delayed by late template parsing without a body.
-  if (!Function->getBody())
-    return;
-
-  // Do not trigger on non-const value parameters when:
-  // 1. they are in a constructor definition since they can likely trigger
-  //    modernize-pass-by-value which will suggest to move the argument.
-  if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) ||
-                            !Function->doesThisDeclarationHaveABody()))
-    return;
-
   auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
-      *Param, *Function->getBody(), *Result.Context);
+      *Param, *Function, *Result.Context);
   auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
-      *Param, *Function->getBody(), *Result.Context);
-  // 2. they are not only used as const.
+      *Param, *Function, *Result.Context);
+
+  // Do not trigger on non-const value parameters when they are not only used as
+  // const.
   if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
     return;
 
@@ -113,20 +104,18 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   // 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) {
+  if (!IsConstQualified && AllDeclRefExprs.size() == 1) {
     auto CanonicalType = Param->getType().getCanonicalType();
-    if (AllDeclRefExprs.size() == 1 &&
-        !hasLoopStmtAncestor(**AllDeclRefExprs.begin(), *Function->getBody(),
-                             *Result.Context) &&
+    const auto &DeclRefExpr  = **AllDeclRefExprs.begin();
+
+    if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
         ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
           utils::decl_ref_expr::isCopyConstructorArgument(
-              **AllDeclRefExprs.begin(), *Function->getBody(),
-              *Result.Context)) ||
+              DeclRefExpr, *Function, *Result.Context)) ||
          (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
           utils::decl_ref_expr::isCopyAssignmentArgument(
-              **AllDeclRefExprs.begin(), *Function->getBody(),
-              *Result.Context)))) {
-      handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context);
+              DeclRefExpr, *Function, *Result.Context)))) {
+      handleMoveFix(*Param, DeclRefExpr, *Result.Context);
       return;
     }
   }