[clang-tidy] Do not move parameter if only DeclRefExpr occurs inside of a loop

[flx]

Summary

This fixes a bug where the performance-unnecessary-value-param check suggests a fix to move the parameter inside of a loop which could be invoked multiple times.

[alexfh]

clang-tidy/utils/DeclRefExprUtils.cpp

127

How will this work with lambdas / local classes declared inside a loop? Not sure if this case is going to happen in real code, but we'd better be clear about the limitations of the implementation.

129

Do you actually need to bind the node to "declRef"?

[flx]

clang-tidy/utils/DeclRefExprUtils.cpp

127

Why would this not work? Could you give an example? The way the function is written it handles my the use case for identifying when moving the parameter is not safe, so I could also just move it into the UnnecessaryValueParamCheck.

[alexfh]

clang-tidy/utils/DeclRefExprUtils.cpp

127

I was thinking about a case where a loop this matcher finds is outside of the function definition and shouldn't be considered:

void F() {
  for (;;) {
    struct C {
      void f(ExpensiveMovableType E) {
        auto F = E;
      }
    };
  }
}

[flx]

clang-tidy/utils/DeclRefExprUtils.cpp

127

This case is not an issue in the check as we're passing f's body statement to the hasLoopStmtAncestor function, so the search is scoped to f's body.

If you're concerned about the case where someone calls this with F's body but expects it to be scoped to f I can just move this function into the check and make it an implementation detail.

[flx]

Do you have any more comments, Alex?

[alexfh]

LG modulo comment.

clang-tidy/utils/DeclRefExprUtils.cpp

127

Making it an implementation detail until other potential users appear sounds good.

[alexfh]

Thanks, this is fine to commit.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   // 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) {
     auto CanonicalType = Param->getType().getCanonicalType();
     if (AllDeclRefExprs.size() == 1 &&
+        !utils::decl_ref_expr::hasLoopStmtAncestor(
+            **AllDeclRefExprs.begin(), *Function->getBody(), *Result.Context) &&
         ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
           utils::decl_ref_expr::isCopyConstructorArgument(
               **AllDeclRefExprs.begin(), *Function->getBody(),
               *Result.Context)) ||
          (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
           utils::decl_ref_expr::isCopyAssignmentArgument(
               **AllDeclRefExprs.begin(), *Function->getBody(),
               *Result.Context)))) {

clang-tidy/utils/DeclRefExprUtils.h

 bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
                                ASTContext &Context);
 
 // Returns true if DeclRefExpr is the argument of a copy-assignment operator
 // call expr.
 bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
                               ASTContext &Context);
 
+// Returns true if DeclRefExpr has a loop statement (ForStmt, CXXForRangeStmt,
+// WhileStmt, DoStmt) as ancestor.
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+                         ASTContext &Context);
+
 } // namespace decl_ref_expr
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_DECLREFEXPRUTILS_H

clang-tidy/utils/DeclRefExprUtils.cpp

bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
   auto Matches = match(
       stmt(hasDescendant(
           cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("="))
               .bind("operatorCallExpr"))),
       Stmt, Context);
   return !Matches.empty();
 }
 
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+                         ASTContext &Context) {
+  auto Matches =
+      match(findAll(declRefExpr(equalsNode(&DeclRef),
+                                unless(hasAncestor(stmt(anyOf(
+                                    forStmt(), cxxForRangeStmt(), whileStmt(),
+                                    doStmt())))))),
+            Stmt, Context);
+  return Matches.empty();
+}
+
 } // namespace decl_ref_expr
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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

 
 void PositiveMoveOnCopyAssignment(ExpensiveMovableType E) {
   ExpensiveMovableType F;
   F = E;
   // CHECK-MESSAGES: [[@LINE-1]]:7: warning: parameter 'E' is passed by value
   // CHECK-FIXES: F = std::move(E);
 }
 
+// The argument could be moved but is not since copy statement is inside a loop.
+void PositiveNoMoveInsideLoop(ExpensiveMovableType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the parameter 'E' is copied
+  // CHECK-FIXES: void PositiveNoMoveInsideLoop(const ExpensiveMovableType& E) {
+  for (;;) {
+    auto F = E;
+  }
+}
+
 void PositiveConstRefNotMoveConstructible(ExpensiveToCopyType T) {
   // CHECK-MESSAGES: [[@LINE-1]]:63: warning: the parameter 'T' is copied
   // CHECK-FIXES: void PositiveConstRefNotMoveConstructible(const ExpensiveToCopyType& T) {
   auto U = T;
 }
 
 void PositiveConstRefNotMoveAssignable(ExpensiveToCopyType A) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the parameter 'A' is copied