Author: serge-sans-paille Date: 2026-03-02T06:21:58Z New Revision: 6d82f143dee1add67f64ceca89a7bce7bcf2277a
URL: https://github.com/llvm/llvm-project/commit/6d82f143dee1add67f64ceca89a7bce7bcf2277a DIFF: https://github.com/llvm/llvm-project/commit/6d82f143dee1add67f64ceca89a7bce7bcf2277a.diff LOG: [clang-tidy] New performance linter: performance-use-std-move (#179467) This linter suggests calls to ``std::move`` when a costly copy would happen otherwise. It does not try to suggest ``std::move`` when they are valid but obviously not profitable (e.g. for trivially movable types) This is a very simple version that only considers terminating basic blocks. Further work will extend the approach through the control flow graph. It has already been used successfully on llvm/lib to submit bugs #178174, #178169, #178176, #178172, #178175, #178180, #178178, #178177, #178179, #178173 and #178167, and on the firefox codebase to submit most of the dependencies of bug https://bugzilla.mozilla.org/show_bug.cgi?id=2012658 Added: clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp Modified: clang-tools-extra/clang-tidy/performance/CMakeLists.txt clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index 4dba117e1ee54..5b3e849fb20a8 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -26,6 +26,7 @@ add_clang_library(clangTidyPerformanceModule STATIC TypePromotionInMathFnCheck.cpp UnnecessaryCopyInitializationCheck.cpp UnnecessaryValueParamCheck.cpp + UseStdMoveCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index 294a209e4c602..21274fae43795 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -28,6 +28,7 @@ #include "TypePromotionInMathFnCheck.h" #include "UnnecessaryCopyInitializationCheck.h" #include "UnnecessaryValueParamCheck.h" +#include "UseStdMoveCheck.h" namespace clang::tidy { namespace performance { @@ -73,6 +74,7 @@ class PerformanceModule : public ClangTidyModule { "performance-unnecessary-copy-initialization"); CheckFactories.registerCheck<UnnecessaryValueParamCheck>( "performance-unnecessary-value-param"); + CheckFactories.registerCheck<UseStdMoveCheck>("performance-use-std-move"); } }; diff --git a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp new file mode 100644 index 0000000000000..d6be91b86eb8c --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp @@ -0,0 +1,122 @@ +//===--- UseStdMoveCheck.cpp - clang-tidy ---------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "UseStdMoveCheck.h" + +#include "../utils/DeclRefExprUtils.h" + +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/STLExtras.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::performance { + +namespace { +AST_MATCHER(CXXRecordDecl, hasNonTrivialMoveAssignment) { + return Node.hasNonTrivialMoveAssignment(); +} + +AST_MATCHER(QualType, isLValueReferenceType) { + return Node->isLValueReferenceType(); +} + +AST_MATCHER(DeclRefExpr, refersToEnclosingVariableOrCapture) { + return Node.refersToEnclosingVariableOrCapture(); +} + +AST_MATCHER(CXXOperatorCallExpr, isCopyAssignmentOperator) { + if (const auto *MD = dyn_cast_or_null<CXXMethodDecl>(Node.getDirectCallee())) + return MD->isCopyAssignmentOperator(); + return false; +} + +// Ignore nodes inside macros. +AST_POLYMORPHIC_MATCHER(isInMacro, + AST_POLYMORPHIC_SUPPORTED_TYPES(Stmt, Decl)) { + return Node.getBeginLoc().isMacroID() || Node.getEndLoc().isMacroID(); +} +} // namespace + +using utils::decl_ref_expr::allDeclRefExprs; + +void UseStdMoveCheck::registerMatchers(MatchFinder *Finder) { + auto AssignOperatorExpr = + cxxOperatorCallExpr( + isCopyAssignmentOperator(), + hasArgument(0, hasType(cxxRecordDecl(hasNonTrivialMoveAssignment()))), + hasArgument( + 1, declRefExpr( + to(varDecl( + hasLocalStorage(), + hasType(qualType(unless(anyOf( + isLValueReferenceType(), + isConstQualified() // Not valid to move const obj. + )))))), + unless(refersToEnclosingVariableOrCapture())) + .bind("assign-value")), + forCallable(functionDecl().bind("within-func")), unless(isInMacro())) + .bind("assign"); + Finder->addMatcher(AssignOperatorExpr, this); +} + +const CFG *UseStdMoveCheck::getCFG(const FunctionDecl *FD, + ASTContext *Context) { + std::unique_ptr<CFG> &TheCFG = CFGCache[FD]; + if (!TheCFG) { + const CFG::BuildOptions Options; + std::unique_ptr<CFG> FCFG = + CFG::buildCFG(nullptr, FD->getBody(), Context, Options); + if (!FCFG) + return nullptr; + TheCFG.swap(FCFG); + } + return TheCFG.get(); +} + +void UseStdMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *AssignExpr = Result.Nodes.getNodeAs<Expr>("assign"); + const auto *AssignValue = Result.Nodes.getNodeAs<DeclRefExpr>("assign-value"); + const auto *WithinFunctionDecl = + Result.Nodes.getNodeAs<FunctionDecl>("within-func"); + + const CFG *TheCFG = getCFG(WithinFunctionDecl, Result.Context); + if (!TheCFG) + return; + + // Walk the CFG bottom-up, starting with the exit node. + // TODO: traverse the whole CFG instead of only considering terminator + // nodes. + const CFGBlock &TheExit = TheCFG->getExit(); + for (auto &Pred : TheExit.preds()) { + if (!Pred.isReachable()) + continue; + for (const CFGElement &Elt : llvm::reverse(*Pred)) { + if (Elt.getKind() != CFGElement::Kind::Statement) + continue; + + const Stmt *EltStmt = Elt.castAs<CFGStmt>().getStmt(); + if (EltStmt == AssignExpr) { + diag(AssignValue->getBeginLoc(), "'%0' could be moved here") + << AssignValue->getDecl()->getName(); + break; + } + // The reference is being referenced after the assignment, bail out. + if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt, + *Result.Context) + .empty()) + break; + } + } +} + +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h new file mode 100644 index 0000000000000..0bc1543d6e372 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h @@ -0,0 +1,35 @@ +//===--- InefficientCopyAssign.h - clang-tidy -----------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H + +#include "../ClangTidyCheck.h" + +#include "clang/Analysis/CFG.h" + +namespace clang::tidy::performance { + +class UseStdMoveCheck : public ClangTidyCheck { +public: + UseStdMoveCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + llvm::DenseMap<const FunctionDecl *, std::unique_ptr<CFG>> CFGCache; + const CFG *getCFG(const FunctionDecl *, ASTContext *); +}; + +} // namespace clang::tidy::performance + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6bdc0ae7bdcc8..9aef286eedc30 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -142,6 +142,12 @@ New checks Finds and removes redundant conversions from ``std::[w|u8|u16|u32]string_view`` to ``std::[...]string`` in call expressions expecting ``std::[...]string_view``. +- New :doc:`performance-use-std-move + <clang-tidy/checks/performance/use-std-move>` check. + + Suggests insertion of ``std::move(...)`` to turn copy assignment operator + calls into move assignment ones, when deemed valid and profitable. + - New :doc:`readability-trailing-comma <clang-tidy/checks/readability/trailing-comma>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c475870ed7b31..24086a4637dbf 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -369,6 +369,7 @@ Clang-Tidy Checks :doc:`performance-type-promotion-in-math-fn <performance/type-promotion-in-math-fn>`, "Yes" :doc:`performance-unnecessary-copy-initialization <performance/unnecessary-copy-initialization>`, "Yes" :doc:`performance-unnecessary-value-param <performance/unnecessary-value-param>`, "Yes" + :doc:`performance-use-std-move <performance/use-std-move>`, :doc:`portability-avoid-pragma-once <portability/avoid-pragma-once>`, :doc:`portability-restrict-system-includes <portability/restrict-system-includes>`, "Yes" :doc:`portability-simd-intrinsics <portability/simd-intrinsics>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst new file mode 100644 index 0000000000000..659126b19c504 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst @@ -0,0 +1,23 @@ +.. title:: clang-tidy - performance-use-std-move + +performance-use-std-move +======================== + +Suggests insertion of ``std::move(...)`` to turn copy assignment operator calls +into move assignment ones, when deemed valid and profitable. + +.. code-block:: c++ + + void assign(std::vector<int>& out) { + std::vector<int> some = make_vector(); + use_vector(some); + out = some; + } + + // becomes + + void assign(std::vector<int>& out) { + std::vector<int> some = make_vector(); + use_vector(some); + out = std::move(some); + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp new file mode 100644 index 0000000000000..d823fa75720af --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp @@ -0,0 +1,288 @@ +// RUN: %check_clang_tidy %s performance-use-std-move %t + +// Definitions used in the tests +// ----------------------------- + +namespace std { +template<class T> struct remove_reference { typedef T type; }; +template<class T> struct remove_reference<T&> { typedef T type; }; +template<class T> struct remove_reference<T&&> { typedef T type; }; +template< class T > +constexpr typename remove_reference<T>::type&& move( T&& t ) noexcept; +} + +struct NonTrivialMoveAssign { + NonTrivialMoveAssign() = default; + NonTrivialMoveAssign(const NonTrivialMoveAssign&) = default; + + NonTrivialMoveAssign& operator=(const NonTrivialMoveAssign&); + NonTrivialMoveAssign& operator=(NonTrivialMoveAssign&&); + + NonTrivialMoveAssign& operator+=(const NonTrivialMoveAssign&); + NonTrivialMoveAssign& operator+=(NonTrivialMoveAssign&&); + void stuff(); +}; + +struct TrivialMoveAssign { + TrivialMoveAssign& operator=(const TrivialMoveAssign&); + TrivialMoveAssign& operator=(TrivialMoveAssign&&) = default; +}; + +struct NoMoveAssign { + NoMoveAssign& operator=(const NoMoveAssign&); + NoMoveAssign& operator=(NoMoveAssign&&) = delete; +}; + +template<class T> +void use(T&) {} + +// Check moving from various reference/pointer type +// ------------------------------------------------ + +void ConvertibleNonTrivialMoveAssign(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +void NonProfitableNonTrivialMoveAssignPointer(NonTrivialMoveAssign*& target, NonTrivialMoveAssign* source) { + // No message expected, moving is possible but non profitable for pointer. + target = source; +} + +void ConvertibleNonTrivialMoveAssignFromLValue(NonTrivialMoveAssign& target, NonTrivialMoveAssign&& source) { + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +// Check moving already moved values +// --------------------------------- + +void NonConvertibleNonTrivialMoveAssignAlreadyMoved(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // No message expected, it's already moved + target = std::move(source); +} + +void NonConvertibleNonTrivialMoveAssignFromLValueAlreadyMoved(NonTrivialMoveAssign& target, NonTrivialMoveAssign&& source) { + // No message expected, it's already moved + target = std::move(source); +} + +// Check moving within diff erent context +// ------------------------------------- + +struct SomeRecord { +void ConvertibleNonTrivialMoveAssignWithinMethod(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} +}; + +auto ConvertibleNonTrivialMoveAssignWithinLambda = [](NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +}; + +void SomeFunction(NonTrivialMoveAssign source0, NonTrivialMoveAssign const &source1) { + +auto NonConvertibleNonTrivialMoveAssignWithinLambdaAsCaptureByRef = [&](NonTrivialMoveAssign& target) { + // No message expected, cannot move a non-local variable. + target = source0; + // No message expected, cannot move a non-local variable. + target = source1; +}; + +auto ConvertibleNonTrivialMoveAssignWithinLambdaAsCapture = [=](NonTrivialMoveAssign& target) { + // No message expected, cannot move a non-local variable. + target = source0; + // No message expected, cannot move a non-local variable. + target = source1; +}; + +} + +void ConvertibleNonTrivialMoveAssignShadowing(NonTrivialMoveAssign& target, NoMoveAssign source) { + { + NonTrivialMoveAssign source; + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] + target = source; + } +} + +void ConvertibleNonTrivialMoveAssignShadowed(NoMoveAssign& target, NonTrivialMoveAssign source) { + { + NoMoveAssign source; + // No message expected, `source' refers to a non-movable variable. + target = source; + } +} + +#define ASSIGN(x, y) x = y +void ConvertibleNonTrivialMoveAssignWithinMacro(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // No message expected, assignment within a macro. + ASSIGN(target, source); +} + +template<class T> +void ConvertibleNonTrivialMoveAssignWithTemplateSource(NonTrivialMoveAssign& target, T source) { + // No message expected, type of source cannot be matched against `target's type. + target = source; +} + +template<class T> +void ConvertibleNonTrivialMoveAssignWithTemplateTarget(T& target, NonTrivialMoveAssign source) { + // No message expected, type of target cannot be matched against `source's type. + target = source; +} + +// Check moving from various storage +// --------------------------------- + +void NonConvertibleNonTrivialMoveAssignFromLocal(NonTrivialMoveAssign& target) { + const NonTrivialMoveAssign source; + // No message, moving a const-qualified value is not valid. + target = source; +} + +void NonConvertibleNonTrivialMoveAssignFromConst(NonTrivialMoveAssign& target) { + NonTrivialMoveAssign source; + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) { + static NonTrivialMoveAssign source; + // No message, the lifetime of `source' does not end with the scope of the function. + target = source; +} + +struct NonConvertibleNonTrivialMoveAssignFromMember { + NonTrivialMoveAssign source; + void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) { + // No message, `source' is not a local variable. + target = source; + } +}; + +void NonConvertibleNonTrivialMoveAssignFromExtern(NonTrivialMoveAssign& target) { + extern NonTrivialMoveAssign source; + // No message, the lifetime of `source' does not end with the scope of the function. + target = source; +} + +void NonConvertibleNonTrivialMoveAssignFromTLS(NonTrivialMoveAssign& target) { + thread_local NonTrivialMoveAssign source; + // No message, the lifetime of `source' does not end with the scope of the function. + target = source; +} + +NonTrivialMoveAssign global_source; +void NonConvertibleNonTrivialMoveAssignToGlobal(NonTrivialMoveAssign& target) { + // No message, the lifetime of `source' does not end with the scope of the function. + target = global_source; +} + + +// Check moving to various storage +// ------------------------------- + +void ConvertibleNonTrivialMoveAssignToStatic(NonTrivialMoveAssign source) { + static NonTrivialMoveAssign target; + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +struct ConvertibleNonTrivialMoveAssignToMember { + NonTrivialMoveAssign target; + void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] + target = source; + } +}; + +void ConvertibleNonTrivialMoveAssignToExtern(NonTrivialMoveAssign source) { + extern NonTrivialMoveAssign target; + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +void ConvertibleNonTrivialMoveAssignToTLS(NonTrivialMoveAssign source) { + thread_local NonTrivialMoveAssign target; + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +NonTrivialMoveAssign global_target; +void ConvertibleNonTrivialMoveAssignToGlobal(NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:19: warning: 'source' could be moved here [performance-use-std-move] + global_target = source; +} + +void NonConvertibleNonTrivialMoveAssignRValue(NonTrivialMoveAssign& target, NonTrivialMoveAssign const& source) { + // No message expected, moving a reference is invalid there. + target = source; +} + +void NonProfitableTrivialMoveAssign(TrivialMoveAssign& target, TrivialMoveAssign source) { + // No message expected, moving is possible but pedantic. + target = source; +} + +// Check moving in presence of control flow or use +// ----------------------------------------------- + +void ConvertibleNonTrivialMoveAssignWithBranching(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + if(cond) { + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] + target = source; + } +} + +void NonConvertibleNonTrivialMoveAssignWithBranchingAndUse(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + if(cond) { + // No message expected, moving would make use invalid. + target = source; + } + use(source); +} + +void ConvertibleNonTrivialMoveAssignBothBranches(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + if(cond) { + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] + target = source; + } + else { + source.stuff(); + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] + target = source; + } +} + +void NonConvertibleNonTrivialMoveAssignWithExtraUse(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // No message expected, moving would make the call to `stuff' invalid. + target = source; + source.stuff(); +} + +void NonConvertibleNonTrivialMoveAssignInLoop(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // No message expected, moving would make the next iteration invalid. + for(int i = 0; i < 10; ++i) + target = source; +} + +// Check moving for invalid / non profitable type or operation +// ----------------------------------------------------------- + +void NonConvertibleNonTrivialMoveUpdateAssign(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // No message currently expected, we only consider assignment. + target += source; +} + +void NonProfitableTrivialTypeAssign(int& target, int source) { + // No message needed, it's correct to move but pedantic. + target = source; +} + +void InvalidMoveAssign(NoMoveAssign& target, NoMoveAssign source) { + // No message expected, moving is deleted. + target = source; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
