https://github.com/serge-sans-paille created https://github.com/llvm/llvm-project/pull/179467
This linter suggests call 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 >From 875018aaa2735fbd299cbe74c1e1c7dfca6ec153 Mon Sep 17 00:00:00 2001 From: serge-sans-paille <[email protected]> Date: Tue, 27 Jan 2026 12:47:35 +0100 Subject: [PATCH] [clang-tidy] New performance linter: performance-inefficient-copy-assign This linter suggests call 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 ---------- _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
