[clang-tidy] unnecessary-value-param: Allow moving of value arguments.

[legrosbuffle]

Some functions take an argument by value because it allows efficiently moving them to a function that takes by value. Do not pessimize that case.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Clement Courbet (legrosbuffle)

Changes

Some functions take an argument by value because it allows efficiently moving them to a function that takes by value. Do not pessimize that case.

---

Full diff: https://github.com/llvm/llvm-project/pull/145871.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+16)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+1)
• (modified) clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp (+4)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index d89c3a69fc841..d18a3c914bf93 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -41,6 +41,17 @@ bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Decl &Decl,
   return Matches.empty();
 }
 
+bool isArgOfStdMove(const DeclRefExpr &DeclRef, const Decl &Decl,
+                    ASTContext &Context) {
+  auto Matches = match(
+      traverse(TK_AsIs, decl(forEachDescendant(callExpr(
+                            callee(functionDecl(hasName("std::move"))),
+                            hasArgument(0, ignoringParenImpCasts(declRefExpr(
+                                               equalsNode(&DeclRef)))))))),
+      Decl, Context);
+  return Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -100,6 +111,11 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
       auto CanonicalType = Param->getType().getCanonicalType();
       const auto &DeclRefExpr = **AllDeclRefExprs.begin();
 
+      // The reference is in a call to `std::move`, do not warn.
+      if (isArgOfStdMove(DeclRefExpr, *Function, *Result.Context)) {
+        return;
+      }
+
       if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
           ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
             utils::decl_ref_expr::isCopyConstructorArgument(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 934d52086b3b9..2413b742dc480 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -294,6 +294,7 @@ Changes in existing checks
   to avoid matching usage of functions within the current compilation unit.
   Added an option `IgnoreCoroutines` with the default value `true` to
   suppress this check for coroutines where passing by reference may be unsafe.
+  Fix false positive on by-value parameters that are only moved.
 
 - Improved :doc:`readability-convert-member-functions-to-static
   <clang-tidy/checks/readability/convert-member-functions-to-static>` check by
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
index 88c491ea1eabc..a9af4f60561a4 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
@@ -344,6 +344,10 @@ void ReferenceFunctionByCallingIt() {
   PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
 }
 
+void NegativeMoved(ExpensiveToCopyType A) {
+  A Copy = std::move(A);
+}
+
 // Virtual method overrides of dependent types cannot be recognized unless they
 // are marked as override or final. Test that check is not triggered on methods
 // marked with override or final.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Clement Courbet (legrosbuffle)

Changes

Some functions take an argument by value because it allows efficiently moving them to a function that takes by value. Do not pessimize that case.

---

Full diff: https://github.com/llvm/llvm-project/pull/145871.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+16)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+1)
• (modified) clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp (+4)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index d89c3a69fc841..d18a3c914bf93 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -41,6 +41,17 @@ bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Decl &Decl,
   return Matches.empty();
 }
 
+bool isArgOfStdMove(const DeclRefExpr &DeclRef, const Decl &Decl,
+                    ASTContext &Context) {
+  auto Matches = match(
+      traverse(TK_AsIs, decl(forEachDescendant(callExpr(
+                            callee(functionDecl(hasName("std::move"))),
+                            hasArgument(0, ignoringParenImpCasts(declRefExpr(
+                                               equalsNode(&DeclRef)))))))),
+      Decl, Context);
+  return Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
@@ -100,6 +111,11 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
       auto CanonicalType = Param->getType().getCanonicalType();
       const auto &DeclRefExpr = **AllDeclRefExprs.begin();
 
+      // The reference is in a call to `std::move`, do not warn.
+      if (isArgOfStdMove(DeclRefExpr, *Function, *Result.Context)) {
+        return;
+      }
+
       if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
           ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
             utils::decl_ref_expr::isCopyConstructorArgument(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 934d52086b3b9..2413b742dc480 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -294,6 +294,7 @@ Changes in existing checks
   to avoid matching usage of functions within the current compilation unit.
   Added an option `IgnoreCoroutines` with the default value `true` to
   suppress this check for coroutines where passing by reference may be unsafe.
+  Fix false positive on by-value parameters that are only moved.
 
 - Improved :doc:`readability-convert-member-functions-to-static
   <clang-tidy/checks/readability/convert-member-functions-to-static>` check by
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
index 88c491ea1eabc..a9af4f60561a4 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param.cpp
@@ -344,6 +344,10 @@ void ReferenceFunctionByCallingIt() {
   PositiveMessageAndFixAsFunctionIsCalled(ExpensiveToCopyType());
 }
 
+void NegativeMoved(ExpensiveToCopyType A) {
+  A Copy = std::move(A);
+}
+
 // Virtual method overrides of dependent types cannot be recognized unless they
 // are marked as override or final. Test that check is not triggered on methods
 // marked with override or final.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vbvictor]

Shouldn't this case be already covered by

void PositiveMoveOnCopyConstruction(ExpensiveMovableType E) {
  auto F = E;
  // CHECK-MESSAGES: [[@LINE-1]]:12: warning: parameter 'E' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
  // CHECK-FIXES: auto F = std::move(E);
}

and

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;
}

We have a difference when the check would suggest move or const&

[vbvictor]

If we want to change the behavior of the check, I'd suggest making an option that will force "always move", no matter how expensive is the object to copy

[legrosbuffle]

Shouldn't this case be already covered by

No, this case is different: note that the std::move is already present.

With

void NegativeMoved(ExpensiveToCopyType A) {
  ExpensiveToCopyType Copy = std::move(A);
}

If the caller passes an lvalue (case L), there is one copy (in the caller) and one move (in NegativeMoved). If the caller passes an rvalue (case R), there are two moves: one in the caller, and on in NegativeMoved.

The check currently suggests:

void NegativeMoved(const ExpensiveToCopyType& A) {
  ExpensiveToCopyType Copy = std::move(A);  // Note: move becomes useless.
}

With this code, we always have a copy in both cases L and R, because std::move yields a const&&, which selects the copy ctor. There is no way for the caller to benefit from having an rvalue.

Given that move is typically much cheaper than copy, there should never be a reason to pick the second version, especially given tha the user expressed their intent to move.

[vbvictor]

To sum up:
With std::move it's 1 copy 1 move for lvalue or 1 move for rvalue
With const& it's 1 copy for lvalue or rvalue
small repro https://godbolt.org/z/EosczKoo7

For me it this patch adds inconsistency for the check: if we have

void NegativeMoved(ExpensiveToCopyType A) {
  ExpensiveToCopyType Copy = std::move(A); // no warning
}

when I remove std::move, I'd expect the check to suggest it, but now it wants const&

void NegativeMoved(ExpensiveToCopyType A) { // use const &
  ExpensiveToCopyType Copy = A;
}

I think a better way is to provide the user an option to specify that he always want to use move. WDYT? reviewers

[vbvictor]

Sorry for accidentally closing, my mouse slipped:(

[firewave]

The existing behavior is already somewhat inconsistent based on the code: #57908. See also #94798.

[legrosbuffle]

Note that in the test I wrote I called a ctor for simplicity, but in general I want to avoid warning on anything that looks like this:

template <typename... Args>
void Eat(Args&&...);

void NegativeMoved(ExpensiveToCopyType A) {
  Eat(std::move(A));
}

The move ctor just happens to be a specific case of Eat.

If we wanted to be fully consistent we'd have to turn:

void NegativeMoved(ExpensiveToCopyType A) {
  Eat(A);
}

into:

void NegativeMoved(ExpensiveToCopyType A) {
  Eat(std::move(A));
}

We've actually had such a check internally for a few years (suggesting to move local objects, including parameters). This check is extremely complex (though it handles more cases than just a single reference to the parameter to make a copy, in particular it handles the case that @firewave mentions in #57908. So I think suggesting the move is way beyond the scope of unnecessary-value-param. unnecessary-value-param happens to suggest the move in the very specific case of "a by-value parameter used exactly once as the argument of a copy constructor except if we're anywhere within a loop" , but that's a bit ad hoc and just there because it's a common pattern (in particular, in constructor init lists) that we don't want to pessimize.

[firewave]

We've actually had such a check internally for a few years (suggesting to move local objects, including parameters).

See #53489 and #139525 about such a check.

I think moving (no pun intended) the logic for suggesting using std::move() into a separate check is an interested point so there would be no overlap.

This is also the case with other checks which combine multiple functionalities - which makes sense if you want a "standalone" check which gives you the final state of the fixed code. But it might lead to aforementioned overlap and duplicated logic, and at some point possibly even inconsistencies. IMO only having incremental changes and requiring multiple runs until you have clean code is fine.