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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
