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

[malcolm.parsons]

Summary

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

[alexfh]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

114 ↗

(On Diff #82230)

How about pulling out variables for **AllDeclRefExprs.begin() and *Function->getDefinition() to remove some boilerplate?

clang-tidy/utils/DeclRefExprUtils.h

35 ↗

(On Diff #82230)

Please convert the comment (and the other ones the patch touches) to doxygen style.

55–56 ↗

(On Diff #82230)

The comment is now confusing. It says nothing about Decl and doesn't make it clear where the "call expr" (btw, it should either be CallExpr, if it references to the corresponding type or "call expression" otherwise) comes from. Same above.

[malcolm.parsons]

Add some variables.
Improve comments.

[aaron.ballman]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

Instead of using hasBody() and getDefinition(), you should use FunctionDecl::getBody() and pass in an argument to receive the function's definition.

[malcolm.parsons]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

I don't want the body - the CXXCtorInitializers are not in it.

[malcolm.parsons]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

I should use hasBody() and pass in an argument.

[aaron.ballman]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

You are calling Function-hasBody() followed by Function->getDefinition(), which is what FunctionDecl::getBody() does. The difference is that FunctionDecl::getBody() will also load serialized bodies from the AST (if the body happens to be in a PCH, for instance).

[malcolm.parsons]

Move hasBody check into matcher.
The matcher checks the function is a definition.

[malcolm.parsons]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

Do I want to load serialized bodies?

[aaron.ballman]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

I would imagine you would want to deserialize, since you care about the contents of the body, not just the presence of one. This may or may not be important with modules (Richard Smith would be able to give a more definitive answer there).

[alexfh]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

I would suggest writing minimal sane code that works until we have good reasons to write more complex code to support module-enabled builds (and appropriate tests).

clang-tidy/utils/DeclRefExprUtils.h

51 ↗

(On Diff #82239)

Please enclose true, DeclRefExpr and other inline code snippets in double backquotes.

[aaron.ballman]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

96 ↗

(On Diff #82237)

I think either approach is minimal, sane code and we should understand why we shouldn't use the interface provided by FunctionDecl::getBody() for getting the definition before deciding to not use it.

I *think* we're fine to not use getBody() because the matcher should match over all of the function declarations with a definition, and so eventually it will find the definition with the actual body. In the case that the function definition is serialized, I think this already does the right thing and deserializes it. (I think the only case where we need the deserialization may be when we have a function declaration and we need to traverse from that to the function body, which may be in a different FunctionDecl.) So the use of hasBody() in the matcher above seems like the correct solution to me.

[malcolm.parsons]

Add double backticks.

[alexfh]

LG

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

 bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function,
                                    ASTContext &Context) {
   auto Matches = match(declRefExpr(to(functionDecl(equalsNode(&Function))),
                                    unless(hasAncestor(callExpr()))),
                        Context);
   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();
 }
 
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
           Options.get("IncludeStyle", "llvm"))) {}
 
 void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
   const auto ExpensiveValueParamDecl =
       parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(),
                                                  unless(referenceType())))),
                   decl().bind("param"));
   Finder->addMatcher(
-      functionDecl(isDefinition(),
+      functionDecl(hasBody(stmt()), isDefinition(),
                    unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
                    unless(isInstantiated()),
                    has(typeLoc(forEach(ExpensiveValueParamDecl))),
                    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();
 
-  // 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;
 
   // 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) {
+  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;
     }
   }
 
   auto Diag =
       diag(Param->getLocation(),
            IsConstQualified ? "the const qualified parameter %0 is "
                               "copied for each invocation; consider "

clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h

 /// do not modify it.
 ///
 /// Returns ``true`` if only const methods or operators are called on the
 /// variable or the variable is a const reference or value argument to a
 /// ``callExpr()``.
 bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
                        ASTContext &Context);
 
-// Returns set of all DeclRefExprs to VarDecl in Stmt.
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
 llvm::SmallPtrSet<const DeclRefExpr *, 16>
 allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
 
-// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
-// a const fashion.
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl``.
+llvm::SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
+
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where
+/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
 llvm::SmallPtrSet<const DeclRefExpr *, 16>
 constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
                            ASTContext &Context);
 
-// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl`` where
+/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
+llvm::SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl,
+                           ASTContext &Context);
+
+/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-constructor
+/// call expression within ``Decl``.
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
                                ASTContext &Context);
 
-// Returns true if DeclRefExpr is the argument of a copy-assignment operator
-// call expr.
-bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-assignment
+/// operator CallExpr within ``Decl``.
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
                               ASTContext &Context);
 
 } // namespace decl_ref_expr
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_DECLREFEXPRUTILS_H

clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp

constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =
       match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   return DeclRefs;
 }
 
+// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
+// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
+SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl,
+                           ASTContext &Context) {
+  auto DeclRefToVar =
+      declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+  auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
+  // Match method call expressions where the variable is referenced as the this
+  // implicit object argument and opertor call expression for member operators
+  // where the variable is the 0-th argument.
+  auto Matches =
+      match(decl(forEachDescendant(expr(
+                anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
+                      cxxOperatorCallExpr(ConstMethodCallee,
+                                          hasArgument(0, DeclRefToVar)))))),
+            Decl, Context);
+  SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  auto ConstReferenceOrValue =
+      qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))),
+                     unless(anyOf(referenceType(), pointerType()))));
+  auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
+      DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  Matches = match(decl(forEachDescendant(callExpr(UsedAsConstRefOrValueArg))),
+                  Decl, Context);
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  Matches =
+      match(decl(forEachDescendant(cxxConstructExpr(UsedAsConstRefOrValueArg))),
+            Decl, Context);
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  return DeclRefs;
+}
+
 bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
                        ASTContext &Context) {
   // Collect all DeclRefExprs to the loop variable and all CallExprs and
   // CXXConstructExprs where the loop variable is used as argument to a const
   // reference parameter.
   // If the difference is empty it is safe for the loop variable to be a const
   // reference.
   auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
   auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
   return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
 }
 
 SmallPtrSet<const DeclRefExpr *, 16>
 allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) {
   auto Matches = match(
       findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")),
       Stmt, Context);
   SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   return DeclRefs;
 }
 
-bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context) {
+  auto Matches = match(
+      decl(forEachDescendant(
+          declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))),
+      Decl, Context);
+  SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  return DeclRefs;
+}
+
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
                                ASTContext &Context) {
   auto UsedAsConstRefArg = forEachArgumentWithParam(
       declRefExpr(equalsNode(&DeclRef)),
       parmVarDecl(hasType(matchers::isReferenceToConst())));
   auto Matches = match(
-      stmt(hasDescendant(
+      decl(hasDescendant(
           cxxConstructExpr(UsedAsConstRefArg, hasDeclaration(cxxConstructorDecl(
                                                   isCopyConstructor())))
               .bind("constructExpr"))),
-      Stmt, Context);
+      Decl, Context);
   return !Matches.empty();
 }
 
-bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
                               ASTContext &Context) {
   auto UsedAsConstRefArg = forEachArgumentWithParam(
       declRefExpr(equalsNode(&DeclRef)),
       parmVarDecl(hasType(matchers::isReferenceToConst())));
   auto Matches = match(
-      stmt(hasDescendant(
+      decl(hasDescendant(
           cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("="))
               .bind("operatorCallExpr"))),
-      Stmt, Context);
+      Decl, Context);
   return !Matches.empty();
 }
 
 } // namespace decl_ref_expr
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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

void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy) {
   // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter 'ConstCopy'
   // CHECK-MESSAGES: [[@LINE-2]]:120: warning: the parameter 'Copy'
   // CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy) {
 }
 
 struct PositiveConstValueConstructor {
   PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
   // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+  // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
 };
 
 template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
   // CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {

 void negativeValueIsReassigned(ExpensiveToCopyType Obj) {
   Obj = ExpensiveToCopyType();
 }
 
 void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) {
   Obj.nonConstMethod();
 }
 
-struct NegativeValueCopyConstructor {
-  NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+struct PositiveValueUnusedConstructor {
+  PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+  // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {}
+};
+
+struct PositiveValueCopiedConstructor {
+  PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+  // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {}
+  ExpensiveToCopyType Field;
 };
 
 template <typename T>
 struct Container {
   typedef const T & const_reference;
 };
 
 void NegativeTypedefParam(const Container<ExpensiveToCopyType>::const_reference Param) {

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

void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy) {
   // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter 'ConstCopy'
   // CHECK-MESSAGES: [[@LINE-2]]:120: warning: the parameter 'Copy'
   // CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy) {
 }
 
 struct PositiveConstValueConstructor {
   PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
   // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+  // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
 };
 
 template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
   // CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {

 void negativeValueIsReassigned(ExpensiveToCopyType Obj) {
   Obj = ExpensiveToCopyType();
 }
 
 void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) {
   Obj.nonConstMethod();
 }
 
-struct NegativeValueCopyConstructor {
-  NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+struct PositiveValueUnusedConstructor {
+  PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+  // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {}
+};
+
+struct PositiveValueCopiedConstructor {
+  PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+  // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {}
+  ExpensiveToCopyType Field;
+};
+
+struct PositiveValueMovableConstructor {
+  PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(Copy) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:70: warning: parameter 'Copy'
+  // CHECK-FIXES: PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(std::move(Copy)) {}
+  ExpensiveMovableType Field;
+};
+
+struct NegativeValueMovedConstructor {
+  NegativeValueMovedConstructor(ExpensiveMovableType Copy) : Field(static_cast<ExpensiveMovableType &&>(Copy)) {}
+  ExpensiveMovableType Field;
 };
 
 template <typename T>
 struct Container {
   typedef const T & const_reference;
 };
 
 void NegativeTypedefParam(const Container<ExpensiveToCopyType>::const_reference Param) {