[clang-tools-extra] [clang-tidy] unnecessary-value-param: Allow moving of value arguments. (PR #145871)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Clement Courbet (legrosbuffle)

<details>
<summary>Changes</summary>

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
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.

``````````

</details>


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