https://github.com/serge-sans-paille updated https://github.com/llvm/llvm-project/pull/179467
>From 3783a01b5771ea4cb7f890053c4400be23a6e763 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 27 Jan 2026 12:47:35 +0100 Subject: [PATCH 1/8] [clang-tidy] New performance linter: performance-inefficient-copy-assign This linter suggests call 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 --- .../clang-tidy/performance/CMakeLists.txt | 1 + .../InefficientCopyAssignCheck.cpp | 103 ++++++++++++++++++ .../performance/InefficientCopyAssignCheck.h | 35 ++++++ .../performance/PerformanceTidyModule.cpp | 3 + .../performance/inefficient-copy-assign.cpp | 75 +++++++++++++ clang/docs/ReleaseNotes.rst | 6 + 6 files changed, 223 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index 4dba117e1ee54..7b25e5946ddba 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -10,6 +10,7 @@ add_clang_library(clangTidyPerformanceModule STATIC ForRangeCopyCheck.cpp ImplicitConversionInLoopCheck.cpp InefficientAlgorithmCheck.cpp + InefficientCopyAssignCheck.cpp InefficientStringConcatenationCheck.cpp InefficientVectorOperationCheck.cpp MoveConstArgCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp new file mode 100644 index 0000000000000..e3c6cecf82553 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp @@ -0,0 +1,103 @@ +//===--- InefficientCopyAssignCheck.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 "InefficientCopyAssignCheck.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" +#include "llvm/ADT/SmallPtrSet.h" + +#include "../utils/ExprSequence.h" +#include "../utils/Matchers.h" +#include <optional> + +using namespace clang::ast_matchers; +using namespace clang::tidy::utils; + +namespace clang::tidy::performance { + +void InefficientCopyAssignCheck::registerMatchers(MatchFinder *Finder) { + auto AssignOperatorExpr = + cxxOperatorCallExpr( + hasOperatorName("="), + hasArgument( + 0, hasType(cxxRecordDecl(hasMethod(isMoveAssignmentOperator())) + .bind("assign-target-type"))), + hasArgument(1, declRefExpr(to(varDecl().bind("assign-value-decl"))) + .bind("assign-value")), + hasAncestor(functionDecl().bind("within-func"))) + .bind("assign"); + Finder->addMatcher(AssignOperatorExpr, this); +} + +CFG *InefficientCopyAssignCheck::getCFG(const FunctionDecl *FD, + ASTContext *Context) { + std::unique_ptr<CFG> &TheCFG = CFGCache[FD]; + if (!TheCFG) { + 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 InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { + const auto *AssignExpr = Result.Nodes.getNodeAs<Expr>("assign"); + const auto *AssignValue = Result.Nodes.getNodeAs<DeclRefExpr>("assign-value"); + const auto *AssignValueDecl = + Result.Nodes.getNodeAs<VarDecl>("assign-value-decl"); + const auto *AssignTargetType = + Result.Nodes.getNodeAs<CXXRecordDecl>("assign-target-type"); + const auto *WithinFunctionDecl = + Result.Nodes.getNodeAs<FunctionDecl>("within-func"); + + QualType AssignValueQual = AssignValueDecl->getType(); + if (AssignValueQual->isReferenceType() || + AssignValueQual.isConstQualified() || AssignValueQual->isPointerType() || + AssignValueQual->isScalarType()) + return; + + if (AssignTargetType->hasTrivialMoveAssignment()) + return; + + if (CFG *TheCFG = getCFG(WithinFunctionDecl, Result.Context)) { + // Walk the CFG bottom-up, starting with the exit node. + // TODO: traverse the whole CFG instead of only considering terminator + // nodes. + + CFGBlock &TheExit = TheCFG->getExit(); + for (auto &Pred : TheExit.preds()) { + for (const CFGElement &Elt : llvm::reverse(*Pred)) { + if (Elt.getKind() == CFGElement::Kind::Statement) { + 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 before the assignment, bail out. + auto DeclRefMatcher = + declRefExpr(hasDeclaration(equalsNode(AssignValue->getDecl()))); + if (!match(findAll(DeclRefMatcher), *EltStmt, *Result.Context) + .empty()) + break; + } + } + } + } +} + +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h new file mode 100644 index 0000000000000..57cc237152cb6 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.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_INEFFICIENTCOPYASSIGN_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGN_H + +#include "../ClangTidyCheck.h" + +#include "clang/Analysis/CFG.h" + +namespace clang::tidy::performance { + +class InefficientCopyAssignCheck : public ClangTidyCheck { + llvm::DenseMap<const FunctionDecl *, std::unique_ptr<CFG>> CFGCache; + CFG *getCFG(const FunctionDecl *, ASTContext *); + +public: + InefficientCopyAssignCheck(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; +}; + +} // namespace clang::tidy::performance + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGN_H diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index 294a209e4c602..164de2bcba568 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -14,6 +14,7 @@ #include "ForRangeCopyCheck.h" #include "ImplicitConversionInLoopCheck.h" #include "InefficientAlgorithmCheck.h" +#include "InefficientCopyAssignCheck.h" #include "InefficientStringConcatenationCheck.h" #include "InefficientVectorOperationCheck.h" #include "MoveConstArgCheck.h" @@ -46,6 +47,8 @@ class PerformanceModule : public ClangTidyModule { "performance-implicit-conversion-in-loop"); CheckFactories.registerCheck<InefficientAlgorithmCheck>( "performance-inefficient-algorithm"); + CheckFactories.registerCheck<InefficientCopyAssignCheck>( + "performance-inefficient-copy-assign"); CheckFactories.registerCheck<InefficientStringConcatenationCheck>( "performance-inefficient-string-concatenation"); CheckFactories.registerCheck<InefficientVectorOperationCheck>( diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp new file mode 100644 index 0000000000000..ee958bb3479c1 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp @@ -0,0 +1,75 @@ +// RUN: %check_clang_tidy %s performance-inefficient-copy-assign %t + +struct NonTrivialMoveAssign { + 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; +}; + +void ConvertibleNonTrivialMoveAssign(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + target = source; +} + +void NonProfitableTrivialMoveAssign(TrivialMoveAssign& target, TrivialMoveAssign source) { + // No message expected, moving is possible but pedantic. + target = source; +} + +void ConvertibleNonTrivialMoveAssignWithBranching(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + if(cond) { + // CHECK-MESSAGES: [[@LINE+1]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + target = source; + } +} + +void ConvertibleNonTrivialMoveAssignBothBranches(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { + if(cond) { + // CHECK-MESSAGES: [[@LINE+1]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + target = source; + } + else { + source.stuff(); + // CHECK-MESSAGES: [[@LINE+1]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + 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; +} + +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; +} diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 7dc6881ed43e6..b6273a4e4bded 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -354,6 +354,12 @@ Static Analyzer .. _release-notes-sanitizers: +New features +^^^^^^^^^^^^ + +- Added a new checker ``performance.InefficientCopyAssign`` to detect + *copy-assignments* that could profitably be turned into *move-assignments*. + Sanitizers ---------- >From e8d2c120602a94ab9a80b7646dcae217adf3e464 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 3 Feb 2026 15:14:38 +0100 Subject: [PATCH 2/8] fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- .../performance/InefficientCopyAssignCheck.cpp | 9 ++------- .../clang-tidy/performance/InefficientCopyAssignCheck.h | 6 +++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp index e3c6cecf82553..fce7ff38f9201 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp @@ -15,11 +15,6 @@ #include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/SmallPtrSet.h" - -#include "../utils/ExprSequence.h" -#include "../utils/Matchers.h" -#include <optional> using namespace clang::ast_matchers; using namespace clang::tidy::utils; @@ -44,7 +39,7 @@ CFG *InefficientCopyAssignCheck::getCFG(const FunctionDecl *FD, ASTContext *Context) { std::unique_ptr<CFG> &TheCFG = CFGCache[FD]; if (!TheCFG) { - CFG::BuildOptions Options; + const CFG::BuildOptions Options; std::unique_ptr<CFG> FCFG = CFG::buildCFG(nullptr, FD->getBody(), Context, Options); if (!FCFG) @@ -64,7 +59,7 @@ void InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { const auto *WithinFunctionDecl = Result.Nodes.getNodeAs<FunctionDecl>("within-func"); - QualType AssignValueQual = AssignValueDecl->getType(); + const QualType AssignValueQual = AssignValueDecl->getType(); if (AssignValueQual->isReferenceType() || AssignValueQual.isConstQualified() || AssignValueQual->isPointerType() || AssignValueQual->isScalarType()) diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h index 57cc237152cb6..db45c39bb48cd 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h @@ -7,8 +7,8 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGN_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGN_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGNCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGNCHECK_H #include "../ClangTidyCheck.h" @@ -32,4 +32,4 @@ class InefficientCopyAssignCheck : public ClangTidyCheck { } // namespace clang::tidy::performance -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGN_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGNCHECK_H >From 504120b2f684342ce245da85fa651c7a1c1e3dfb Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 3 Feb 2026 15:21:29 +0100 Subject: [PATCH 3/8] fixup! fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- .../clang-tidy/performance/InefficientCopyAssignCheck.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp index fce7ff38f9201..6e75cc4c7b8aa 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp @@ -17,7 +17,6 @@ #include "llvm/ADT/STLExtras.h" using namespace clang::ast_matchers; -using namespace clang::tidy::utils; namespace clang::tidy::performance { >From 34c9e3f0114d404ff11b259a0603c0b05247def8 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 3 Feb 2026 17:49:54 +0100 Subject: [PATCH 4/8] fixup! fixup! fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- .../InefficientCopyAssignCheck.cpp | 8 +- .../performance/InefficientCopyAssignCheck.h | 3 +- clang-tools-extra/docs/ReleaseNotes.rst | 8 +- .../docs/clang-tidy/checks/list.rst | 1 + .../performance/inefficient-copy-assign.rst | 25 +++++ .../performance/inefficient-copy-assign.cpp | 101 +++++++++++++++++- clang/docs/ReleaseNotes.rst | 6 -- 7 files changed, 136 insertions(+), 16 deletions(-) create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp index 6e75cc4c7b8aa..97adf212cbd7a 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp @@ -1,5 +1,4 @@ -//===--- InefficientCopyAssignCheck.cpp - clang-tidy -//-------------------------------===// +//===--- InefficientCopyAssignCheck.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. @@ -59,11 +58,14 @@ void InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { Result.Nodes.getNodeAs<FunctionDecl>("within-func"); const QualType AssignValueQual = AssignValueDecl->getType(); - if (AssignValueQual->isReferenceType() || + if (AssignValueQual->isLValueReferenceType() || AssignValueQual.isConstQualified() || AssignValueQual->isPointerType() || AssignValueQual->isScalarType()) return; + if (!AssignValueDecl->hasLocalStorage()) + return; + if (AssignTargetType->hasTrivialMoveAssignment()) return; diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h index db45c39bb48cd..8684018844155 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h @@ -1,5 +1,4 @@ -//===--- InefficientCopyAssign.h - clang-tidy -//---------------------------------===// +//===--- 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. diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index ea80502735ede..b9565a66fe1f9 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -115,13 +115,19 @@ New checks Looks for functions returning ``std::[w|u8|u16|u32]string`` and suggests to change it to ``std::[...]string_view`` for performance reasons if possible. - + - New :doc:`modernize-use-structured-binding <clang-tidy/checks/modernize/use-structured-binding>` check. Finds places where structured bindings could be used to decompose pairs and suggests replacing them. +- New :doc:`performance-inefficient-copy-assign + <clang-tidy/checks/performance/inefficient-copy-assign>` check. + + Suggests insertion of ``std::move(...)`` to turn copy assignment operator + calls into move assignment ones, when deemed valid and profitable. + - New :doc:`performance-string-view-conversions <clang-tidy/checks/performance/string-view-conversions>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 0eabd9929dc39..d1dff5db3e689 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -354,6 +354,7 @@ Clang-Tidy Checks :doc:`performance-for-range-copy <performance/for-range-copy>`, "Yes" :doc:`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>`, :doc:`performance-inefficient-algorithm <performance/inefficient-algorithm>`, "Yes" + :doc:`performance-inefficient-copy-assign <performance/inefficient-copy-assign>`, "No" :doc:`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>`, :doc:`performance-inefficient-vector-operation <performance/inefficient-vector-operation>`, "Yes" :doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst new file mode 100644 index 0000000000000..10f3008459966 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst @@ -0,0 +1,25 @@ +.. title:: clang-tidy - performance-inefficient-copy-assign + +performance-inefficient-copy-assign +=================================== + + +Warns on copy assignment operator recieving an lvalue reference when a +profitable move assignment operator exist and would be used if the lvalue +reference were moved through ``std::move``. + +.. 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/inefficient-copy-assign.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp index ee958bb3479c1..07e69b48f064c 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp @@ -1,5 +1,8 @@ // RUN: %check_clang_tidy %s performance-inefficient-copy-assign %t +// Definitions used in the tests +// ----------------------------- + struct NonTrivialMoveAssign { NonTrivialMoveAssign& operator=(const NonTrivialMoveAssign&); NonTrivialMoveAssign& operator=(NonTrivialMoveAssign&&); @@ -18,8 +21,84 @@ struct 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]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + 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-inefficient-copy-assign] + target = source; +} + +// Check moving from various storage +// --------------------------------- + +void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) { + static NonTrivialMoveAssign source; + // No message, the lifetime of `source' does not end with the scope of the function. + 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-inefficient-copy-assign] + target = source; +} + +void ConvertibleNonTrivialMoveAssignToExtern(NonTrivialMoveAssign source) { + extern NonTrivialMoveAssign target; + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + target = source; +} + +void ConvertibleNonTrivialMoveAssignToTLS(NonTrivialMoveAssign source) { + thread_local NonTrivialMoveAssign target; + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + target = source; +} + +NonTrivialMoveAssign global_target; +void ConvertibleNonTrivialMoveAssignToGlobal(NonTrivialMoveAssign source) { + // CHECK-MESSAGES: [[@LINE+1]]:19: warning: 'source' could be moved here [performance-inefficient-copy-assign] + global_target = source; +} + +void NonConvertibleNonTrivialMoveAssignRValue(NonTrivialMoveAssign& target, NonTrivialMoveAssign const& source) { + // No message expected, moving a reference is invalid there. target = source; } @@ -28,21 +107,32 @@ void NonProfitableTrivialMoveAssign(TrivialMoveAssign& target, TrivialMoveAssign 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]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-inefficient-copy-assign] 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]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-inefficient-copy-assign] target = source; } else { source.stuff(); - // CHECK-MESSAGES: [[@LINE+1]]:{{[0-9]*}}: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-inefficient-copy-assign] target = source; } } @@ -59,6 +149,9 @@ void NonConvertibleNonTrivialMoveAssignInLoop(NonTrivialMoveAssign& target, NonT 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; diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index b6273a4e4bded..7dc6881ed43e6 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -354,12 +354,6 @@ Static Analyzer .. _release-notes-sanitizers: -New features -^^^^^^^^^^^^ - -- Added a new checker ``performance.InefficientCopyAssign`` to detect - *copy-assignments* that could profitably be turned into *move-assignments*. - Sanitizers ---------- >From bc44e247553a0f77cb5e603dd7d375886c4dc66b Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 3 Feb 2026 18:05:56 +0100 Subject: [PATCH 5/8] fixup! fixup! fixup! fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- .../checkers/performance/inefficient-copy-assign.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp index 07e69b48f064c..13543a65ffd81 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp @@ -45,6 +45,18 @@ void ConvertibleNonTrivialMoveAssignFromLValue(NonTrivialMoveAssign& target, Non // 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-inefficient-copy-assign] + target = source; +} + void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) { static NonTrivialMoveAssign source; // No message, the lifetime of `source' does not end with the scope of the function. >From f9915836d787fc4876785a9adaaaad5c5b05e479 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 3 Feb 2026 21:29:26 +0100 Subject: [PATCH 6/8] fixup! fixup! fixup! fixup! fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- clang-tools-extra/docs/clang-tidy/checks/list.rst | 2 +- .../checks/performance/inefficient-copy-assign.rst | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index d1dff5db3e689..c0dff99213a07 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -354,7 +354,7 @@ Clang-Tidy Checks :doc:`performance-for-range-copy <performance/for-range-copy>`, "Yes" :doc:`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>`, :doc:`performance-inefficient-algorithm <performance/inefficient-algorithm>`, "Yes" - :doc:`performance-inefficient-copy-assign <performance/inefficient-copy-assign>`, "No" + :doc:`performance-inefficient-copy-assign <performance/inefficient-copy-assign>`, :doc:`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>`, :doc:`performance-inefficient-vector-operation <performance/inefficient-vector-operation>`, "Yes" :doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst index 10f3008459966..f84a65dd045e1 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst @@ -3,10 +3,8 @@ performance-inefficient-copy-assign =================================== - -Warns on copy assignment operator recieving an lvalue reference when a -profitable move assignment operator exist and would be used if the lvalue -reference were moved through ``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++ >From cfd82be5b8ec85ccb5f93af5931177f6b57f2e79 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Thu, 5 Feb 2026 22:12:32 +0100 Subject: [PATCH 7/8] fixup! fixup! fixup! fixup! fixup! fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- .../InefficientCopyAssignCheck.cpp | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp index 97adf212cbd7a..3501a96e95409 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp @@ -8,6 +8,8 @@ #include "InefficientCopyAssignCheck.h" +#include "../utils/DeclRefExprUtils.h" + #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -19,6 +21,8 @@ using namespace clang::ast_matchers; namespace clang::tidy::performance { +using utils::decl_ref_expr::allDeclRefExprs; + void InefficientCopyAssignCheck::registerMatchers(MatchFinder *Finder) { auto AssignOperatorExpr = cxxOperatorCallExpr( @@ -26,8 +30,10 @@ void InefficientCopyAssignCheck::registerMatchers(MatchFinder *Finder) { hasArgument( 0, hasType(cxxRecordDecl(hasMethod(isMoveAssignmentOperator())) .bind("assign-target-type"))), - hasArgument(1, declRefExpr(to(varDecl().bind("assign-value-decl"))) - .bind("assign-value")), + hasArgument( + 1, declRefExpr( + to(varDecl(hasLocalStorage()).bind("assign-value-decl"))) + .bind("assign-value")), hasAncestor(functionDecl().bind("within-func"))) .bind("assign"); Finder->addMatcher(AssignOperatorExpr, this); @@ -59,11 +65,7 @@ void InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { const QualType AssignValueQual = AssignValueDecl->getType(); if (AssignValueQual->isLValueReferenceType() || - AssignValueQual.isConstQualified() || AssignValueQual->isPointerType() || - AssignValueQual->isScalarType()) - return; - - if (!AssignValueDecl->hasLocalStorage()) + AssignValueQual.isConstQualified() || AssignValueQual->isScalarType()) return; if (AssignTargetType->hasTrivialMoveAssignment()) @@ -85,9 +87,8 @@ void InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { break; } // The reference is being referenced before the assignment, bail out. - auto DeclRefMatcher = - declRefExpr(hasDeclaration(equalsNode(AssignValue->getDecl()))); - if (!match(findAll(DeclRefMatcher), *EltStmt, *Result.Context) + if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt, + *Result.Context) .empty()) break; } >From b1536d390cf0f3d0e8b6c41262082eedcd078c93 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Fri, 6 Feb 2026 17:23:18 +0100 Subject: [PATCH 8/8] fixup! fixup! fixup! fixup! fixup! fixup! fixup! [clang-tidy] New performance linter: performance-inefficient-copy-assign --- .../clang-tidy/performance/CMakeLists.txt | 2 +- .../performance/PerformanceTidyModule.cpp | 5 +- ...opyAssignCheck.cpp => UseStdMoveCheck.cpp} | 73 +++++++----- ...entCopyAssignCheck.h => UseStdMoveCheck.h} | 17 +-- clang-tools-extra/docs/ReleaseNotes.rst | 12 +- .../docs/clang-tidy/checks/list.rst | 2 +- ...cient-copy-assign.rst => use-std-move.rst} | 6 +- ...cient-copy-assign.cpp => use-std-move.cpp} | 111 ++++++++++++++++-- 8 files changed, 163 insertions(+), 65 deletions(-) rename clang-tools-extra/clang-tidy/performance/{InefficientCopyAssignCheck.cpp => UseStdMoveCheck.cpp} (53%) rename clang-tools-extra/clang-tidy/performance/{InefficientCopyAssignCheck.h => UseStdMoveCheck.h} (67%) rename clang-tools-extra/docs/clang-tidy/checks/performance/{inefficient-copy-assign.rst => use-std-move.rst} (76%) rename clang-tools-extra/test/clang-tidy/checkers/performance/{inefficient-copy-assign.cpp => use-std-move.cpp} (63%) diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index 7b25e5946ddba..5b3e849fb20a8 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -10,7 +10,6 @@ add_clang_library(clangTidyPerformanceModule STATIC ForRangeCopyCheck.cpp ImplicitConversionInLoopCheck.cpp InefficientAlgorithmCheck.cpp - InefficientCopyAssignCheck.cpp InefficientStringConcatenationCheck.cpp InefficientVectorOperationCheck.cpp MoveConstArgCheck.cpp @@ -27,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 164de2bcba568..21274fae43795 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -14,7 +14,6 @@ #include "ForRangeCopyCheck.h" #include "ImplicitConversionInLoopCheck.h" #include "InefficientAlgorithmCheck.h" -#include "InefficientCopyAssignCheck.h" #include "InefficientStringConcatenationCheck.h" #include "InefficientVectorOperationCheck.h" #include "MoveConstArgCheck.h" @@ -29,6 +28,7 @@ #include "TypePromotionInMathFnCheck.h" #include "UnnecessaryCopyInitializationCheck.h" #include "UnnecessaryValueParamCheck.h" +#include "UseStdMoveCheck.h" namespace clang::tidy { namespace performance { @@ -47,8 +47,6 @@ class PerformanceModule : public ClangTidyModule { "performance-implicit-conversion-in-loop"); CheckFactories.registerCheck<InefficientAlgorithmCheck>( "performance-inefficient-algorithm"); - CheckFactories.registerCheck<InefficientCopyAssignCheck>( - "performance-inefficient-copy-assign"); CheckFactories.registerCheck<InefficientStringConcatenationCheck>( "performance-inefficient-string-concatenation"); CheckFactories.registerCheck<InefficientVectorOperationCheck>( @@ -76,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/InefficientCopyAssignCheck.cpp b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp similarity index 53% rename from clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp rename to clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp index 3501a96e95409..4ea3938854d1b 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp @@ -1,4 +1,4 @@ -//===--- InefficientCopyAssignCheck.cpp - clang-tidy ----------------------===// +//===--- 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. @@ -6,7 +6,7 @@ // //===----------------------------------------------------------------------===// -#include "InefficientCopyAssignCheck.h" +#include "UseStdMoveCheck.h" #include "../utils/DeclRefExprUtils.h" @@ -21,26 +21,38 @@ using namespace clang::ast_matchers; namespace clang::tidy::performance { +namespace { +AST_MATCHER(CXXRecordDecl, hasTrivialMoveAssignment) { + return Node.hasTrivialMoveAssignment(); +} + +// 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 InefficientCopyAssignCheck::registerMatchers(MatchFinder *Finder) { +void UseStdMoveCheck::registerMatchers(MatchFinder *Finder) { auto AssignOperatorExpr = cxxOperatorCallExpr( hasOperatorName("="), hasArgument( - 0, hasType(cxxRecordDecl(hasMethod(isMoveAssignmentOperator())) - .bind("assign-target-type"))), + 0, hasType(cxxRecordDecl(hasMethod(isMoveAssignmentOperator()), + unless(hasTrivialMoveAssignment())))), hasArgument( 1, declRefExpr( to(varDecl(hasLocalStorage()).bind("assign-value-decl"))) .bind("assign-value")), - hasAncestor(functionDecl().bind("within-func"))) + hasAncestor(functionDecl().bind("within-func")), unless(isInMacro())) .bind("assign"); Finder->addMatcher(AssignOperatorExpr, this); } -CFG *InefficientCopyAssignCheck::getCFG(const FunctionDecl *FD, - ASTContext *Context) { +const CFG *UseStdMoveCheck::getCFG(const FunctionDecl *FD, + ASTContext *Context) { std::unique_ptr<CFG> &TheCFG = CFGCache[FD]; if (!TheCFG) { const CFG::BuildOptions Options; @@ -53,13 +65,11 @@ CFG *InefficientCopyAssignCheck::getCFG(const FunctionDecl *FD, return TheCFG.get(); } -void InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { +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 *AssignValueDecl = Result.Nodes.getNodeAs<VarDecl>("assign-value-decl"); - const auto *AssignTargetType = - Result.Nodes.getNodeAs<CXXRecordDecl>("assign-target-type"); const auto *WithinFunctionDecl = Result.Nodes.getNodeAs<FunctionDecl>("within-func"); @@ -68,30 +78,29 @@ void InefficientCopyAssignCheck::check(const MatchFinder::MatchResult &Result) { AssignValueQual.isConstQualified() || AssignValueQual->isScalarType()) return; - if (AssignTargetType->hasTrivialMoveAssignment()) + const CFG *TheCFG = getCFG(WithinFunctionDecl, Result.Context); + if (!TheCFG) return; - if (CFG *TheCFG = getCFG(WithinFunctionDecl, Result.Context)) { - // Walk the CFG bottom-up, starting with the exit node. - // TODO: traverse the whole CFG instead of only considering terminator - // nodes. - - CFGBlock &TheExit = TheCFG->getExit(); - for (auto &Pred : TheExit.preds()) { - for (const CFGElement &Elt : llvm::reverse(*Pred)) { - if (Elt.getKind() == CFGElement::Kind::Statement) { - 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 before the assignment, bail out. - if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt, - *Result.Context) - .empty()) - break; + // 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()) { + for (const CFGElement &Elt : llvm::reverse(*Pred)) { + if (Elt.getKind() == CFGElement::Kind::Statement) { + 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 before the assignment, bail out. + if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt, + *Result.Context) + .empty()) + break; } } } diff --git a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h similarity index 67% rename from clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h rename to clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h index 8684018844155..0bc1543d6e372 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientCopyAssignCheck.h +++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h @@ -6,8 +6,8 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGNCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCOPYASSIGNCHECK_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H #include "../ClangTidyCheck.h" @@ -15,20 +15,21 @@ namespace clang::tidy::performance { -class InefficientCopyAssignCheck : public ClangTidyCheck { - llvm::DenseMap<const FunctionDecl *, std::unique_ptr<CFG>> CFGCache; - CFG *getCFG(const FunctionDecl *, ASTContext *); - +class UseStdMoveCheck : public ClangTidyCheck { public: - InefficientCopyAssignCheck(StringRef Name, ClangTidyContext *Context) + 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_INEFFICIENTCOPYASSIGNCHECK_H +#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 b9565a66fe1f9..9d6570b7b6dda 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -122,18 +122,18 @@ New checks Finds places where structured bindings could be used to decompose pairs and suggests replacing them. -- New :doc:`performance-inefficient-copy-assign - <clang-tidy/checks/performance/inefficient-copy-assign>` check. - - Suggests insertion of ``std::move(...)`` to turn copy assignment operator - calls into move assignment ones, when deemed valid and profitable. - - New :doc:`performance-string-view-conversions <clang-tidy/checks/performance/string-view-conversions>` check. 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 c0dff99213a07..61469259f61a0 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -354,7 +354,6 @@ Clang-Tidy Checks :doc:`performance-for-range-copy <performance/for-range-copy>`, "Yes" :doc:`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>`, :doc:`performance-inefficient-algorithm <performance/inefficient-algorithm>`, "Yes" - :doc:`performance-inefficient-copy-assign <performance/inefficient-copy-assign>`, :doc:`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>`, :doc:`performance-inefficient-vector-operation <performance/inefficient-vector-operation>`, "Yes" :doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes" @@ -369,6 +368,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/inefficient-copy-assign.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst similarity index 76% rename from clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst rename to clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst index f84a65dd045e1..659126b19c504 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-copy-assign.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst @@ -1,7 +1,7 @@ -.. title:: clang-tidy - performance-inefficient-copy-assign +.. title:: clang-tidy - performance-use-std-move -performance-inefficient-copy-assign -=================================== +performance-use-std-move +======================== Suggests insertion of ``std::move(...)`` to turn copy assignment operator calls into move assignment ones, when deemed valid and profitable. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp similarity index 63% rename from clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp rename to clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp index 13543a65ffd81..690460e773ea8 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-copy-assign.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp @@ -1,11 +1,15 @@ -// RUN: %check_clang_tidy %s performance-inefficient-copy-assign %t +// RUN: %check_clang_tidy %s performance-use-std-move %t // Definitions used in the tests // ----------------------------- struct NonTrivialMoveAssign { + NonTrivialMoveAssign() = default; + NonTrivialMoveAssign(const NonTrivialMoveAssign&) = default; + NonTrivialMoveAssign& operator=(const NonTrivialMoveAssign&); NonTrivialMoveAssign& operator=(NonTrivialMoveAssign&&); + NonTrivialMoveAssign& operator+=(const NonTrivialMoveAssign&); NonTrivialMoveAssign& operator+=(NonTrivialMoveAssign&&); void stuff(); @@ -28,7 +32,7 @@ void use(T&) {} // ------------------------------------------------ void ConvertibleNonTrivialMoveAssign(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { - // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] target = source; } @@ -38,7 +42,76 @@ void NonProfitableNonTrivialMoveAssignPointer(NonTrivialMoveAssign*& target, Non } void ConvertibleNonTrivialMoveAssignFromLValue(NonTrivialMoveAssign& target, NonTrivialMoveAssign&& source) { - // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] + target = source; +} + +// Check moving within different 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; +}; + +#if 0 +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; +}; + +} +#endif + +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; } @@ -53,7 +126,7 @@ void NonConvertibleNonTrivialMoveAssignFromLocal(NonTrivialMoveAssign& target) { void NonConvertibleNonTrivialMoveAssignFromConst(NonTrivialMoveAssign& target) { NonTrivialMoveAssign source; - // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move] target = source; } @@ -63,6 +136,14 @@ void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) 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. @@ -87,25 +168,33 @@ void NonConvertibleNonTrivialMoveAssignToGlobal(NonTrivialMoveAssign& target) { void ConvertibleNonTrivialMoveAssignToStatic(NonTrivialMoveAssign source) { static NonTrivialMoveAssign target; - // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // 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-inefficient-copy-assign] + // 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-inefficient-copy-assign] + // 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-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:19: warning: 'source' could be moved here [performance-use-std-move] global_target = source; } @@ -124,7 +213,7 @@ void NonProfitableTrivialMoveAssign(TrivialMoveAssign& target, TrivialMoveAssign void ConvertibleNonTrivialMoveAssignWithBranching(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { if(cond) { - // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] target = source; } } @@ -139,12 +228,12 @@ void NonConvertibleNonTrivialMoveAssignWithBranchingAndUse(bool cond, NonTrivial void ConvertibleNonTrivialMoveAssignBothBranches(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) { if(cond) { - // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-inefficient-copy-assign] + // 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-inefficient-copy-assign] + // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move] target = source; } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
