[clang-tools-extra] [llvm] [clang-tidy] Extend performance-use-std-move to copy constructors (PR #228767)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Oct 3 13:46:13 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang-tidy
Author: Vasiliy Kulikov (segoon)
<details>
<summary>Changes</summary>
---
Patch is 82.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/228767.diff
10 Files Affected:
- (modified) clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp (+632-128)
- (modified) clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h (+26-7)
- (modified) clang-tools-extra/docs/ReleaseNotes.md (+6)
- (modified) clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.md (+273-11)
- (added) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move-captures.cpp (+31)
- (added) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move-constrained.cpp (+53)
- (added) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move-construction.cpp (+419)
- (added) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move-flow.cpp (+361)
- (added) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move-options.cpp (+51)
- (modified) clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp (+6-4)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
index e887a9862ca5f..c1138a7ee8b4f 100644
--- a/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp
@@ -7,34 +7,36 @@
//===----------------------------------------------------------------------===//
#include "UseStdMoveCheck.h"
-
-#include "../utils/DeclRefExprUtils.h"
-
+#include "../utils/ExprSequence.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "../utils/TypeTraits.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
+#include "clang/ASTMatchers/ASTMatchersInternal.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Basic/LLVM.h"
#include "clang/Lex/Lexer.h"
-#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include <memory>
+#include <utility>
using namespace clang::ast_matchers;
namespace clang::tidy::performance {
namespace {
-AST_MATCHER(CXXRecordDecl, hasAccessibleNonTrivialMoveAssignment) {
- const CXXRecordDecl *ND = Node.getDefinition();
- if (!ND)
- return false;
- if (!ND->hasNonTrivialMoveAssignment())
- return false;
- for (const CXXMethodDecl *CM : ND->methods())
- if (CM->isMoveAssignmentOperator())
- return !CM->isDeleted() && CM->getAccess() == AS_public;
- llvm_unreachable("Move Assignment Operator Not Found");
-}
-
AST_MATCHER(QualType, isLValueReferenceType) {
return Node->isLValueReferenceType();
}
@@ -43,143 +45,645 @@ AST_MATCHER(DeclRefExpr, refersToEnclosingVariableOrCapture) {
return Node.refersToEnclosingVariableOrCapture();
}
+AST_MATCHER_P(Expr, hasValueSource,
+ ast_matchers::internal::Matcher<DeclRefExpr>, Inner) {
+ const Expr *Value = Node.IgnoreParenImpCasts();
+ while (const auto *Comma = dyn_cast<BinaryOperator>(Value)) {
+ if (Comma->getOpcode() != BO_Comma)
+ break;
+ Value = Comma->getRHS()->IgnoreParenImpCasts();
+ }
+ const auto *Reference = dyn_cast<DeclRefExpr>(Value);
+ return Reference && Inner.matches(*Reference, Finder, Builder);
+}
+
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;
+// Follow wrappers so that, for example, a const reference initializer and a
+// parenthesized reference initializer are treated just like `T &ref = value`.
+static bool
+mayEscape(const DeclRefExpr *Reference, ASTContext &Context,
+ const llvm::DenseMap<const VarDecl *, const VarDecl *> &Aliases) {
+ SmallVector<const Expr *, 4> WorkList{Reference};
+ while (!WorkList.empty()) {
+ const Expr *Expression = WorkList.pop_back_val();
+ for (const DynTypedNode &Parent : Context.getParents(*Expression)) {
+ if (const auto *Variable = Parent.get<VarDecl>()) {
+ if ((Variable->getType()->isReferenceType() &&
+ !Aliases.contains(Variable)) ||
+ Variable->getType()->isPointerType())
+ return true;
+ continue;
+ }
+ const auto *ParentExpr = Parent.get<Expr>();
+ if (!ParentExpr)
+ continue;
+ if (isa<LambdaExpr>(ParentExpr))
+ return true;
+ if (const auto *Init = dyn_cast<InitListExpr>(ParentExpr)) {
+ // The syntactic form can reference the source directly even when the
+ // semantic form contains a value copy. Only the latter binds storage.
+ if (Init->getSemanticForm())
+ continue;
+ // Parent maps can associate a syntactic operand with the semantic
+ // list, bypassing the intervening copy constructor. Only a direct
+ // glvalue or pointer element can retain access to the source.
+ if ((Expression->isGLValue() ||
+ Expression->getType()->isPointerType()) &&
+ llvm::any_of(Init->inits(), [&](const Expr *Element) {
+ return Element && Element->IgnoreParenImpCasts() ==
+ Expression->IgnoreParenImpCasts();
+ }))
+ return true;
+ continue;
+ }
+ if (isa<CXXThrowExpr, CXXNewExpr>(ParentExpr) &&
+ Expression->getType()->isPointerType())
+ return true;
+ if (const auto *Unary = dyn_cast<UnaryOperator>(ParentExpr);
+ Unary && Unary->getOpcode() == UO_AddrOf) {
+ WorkList.push_back(Unary);
+ continue;
+ }
+ if (const auto *Assignment = dyn_cast<BinaryOperator>(ParentExpr);
+ Assignment && Assignment->isAssignmentOp() &&
+ Assignment->getRHS() == Expression &&
+ Expression->getType()->isPointerType())
+ return true;
+ if (isa<BinaryOperator, UnaryOperator>(ParentExpr) &&
+ (ParentExpr->isGLValue() || ParentExpr->getType()->isPointerType())) {
+ WorkList.push_back(ParentExpr);
+ continue;
+ }
+ if (isa<ParenExpr, CastExpr, ExprWithCleanups, MaterializeTemporaryExpr,
+ CXXBindTemporaryExpr, MemberExpr, AbstractConditionalOperator>(
+ ParentExpr)) {
+ WorkList.push_back(ParentExpr);
+ continue;
+ }
+ if (const auto *Construction = dyn_cast<CXXConstructExpr>(ParentExpr)) {
+ const CXXConstructorDecl *Constructor = Construction->getConstructor();
+ if (Constructor->isCopyConstructor())
+ continue;
+ for (unsigned I = 0; I < Construction->getNumArgs(); ++I)
+ if (Construction->getArg(I) == Expression &&
+ (Expression->getType()->isPointerType() ||
+ I >= Constructor->getNumParams() ||
+ Constructor->getParamDecl(I)->getType()->isReferenceType()))
+ return true;
+ }
+ if (const auto *Call = dyn_cast<CallExpr>(ParentExpr)) {
+ if (const auto *Operator = dyn_cast<CXXOperatorCallExpr>(Call);
+ Operator && Operator->getOperator() == OO_Amp &&
+ Operator->getNumArgs() == 1)
+ return true;
+ if (const auto *MemberCall = dyn_cast<CXXMemberCallExpr>(Call);
+ MemberCall &&
+ MemberCall->getImplicitObjectArgument()->IgnoreParenImpCasts() ==
+ Reference &&
+ (MemberCall->isGLValue() || MemberCall->getType()->isPointerType()))
+ WorkList.push_back(MemberCall);
+ const FunctionDecl *Callee = Call->getDirectCallee();
+ if (const auto *Operator = dyn_cast<CXXOperatorCallExpr>(Call);
+ Operator && isa_and_nonnull<CXXMethodDecl>(Callee) &&
+ Operator->getArg(0) == Expression &&
+ (Operator->isGLValue() || Operator->getType()->isPointerType()))
+ WorkList.push_back(Operator);
+ if (const auto *Method = dyn_cast_or_null<CXXMethodDecl>(Callee);
+ Method && Method->isCopyAssignmentOperator())
+ continue;
+ unsigned Offset = isa<CXXOperatorCallExpr>(Call) && Callee &&
+ isa<CXXMethodDecl>(Callee)
+ ? 1
+ : 0;
+ for (unsigned I = Offset; I < Call->getNumArgs(); ++I) {
+ if (Call->getArg(I) != Expression)
+ continue;
+ if (Callee && I - Offset < Callee->getNumParams() &&
+ Callee->getParamDecl(I - Offset)->hasAttr<NoEscapeAttr>())
+ continue;
+ // Unknown callees may retain a reference to the argument.
+ if (Expression->getType()->isPointerType() || !Callee ||
+ I - Offset >= Callee->getNumParams() ||
+ Callee->getParamDecl(I - Offset)->getType()->isReferenceType())
+ return true;
+ }
+ }
+ }
+ }
+ return false;
+}
-void UseStdMoveCheck::registerMatchers(MatchFinder *Finder) {
- const auto AssignOperatorExpr =
- cxxOperatorCallExpr(
- isCopyAssignmentOperator(),
- hasArgument(0, hasType(cxxRecordDecl(
- hasAccessibleNonTrivialMoveAssignment()))),
- 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)
+static const VarDecl *initializingVariable(const Expr *Expression,
+ ASTContext &Context) {
+ while (true) {
+ const DynTypedNodeList Parents = Context.getParents(*Expression);
+ if (Parents.size() != 1)
+ return nullptr;
+ if (const auto *Variable = Parents[0].get<VarDecl>())
+ return Variable;
+ const auto *Parent = Parents[0].get<Expr>();
+ if (!Parent || !isa<ExprWithCleanups, CXXBindTemporaryExpr,
+ MaterializeTemporaryExpr, ImplicitCastExpr>(Parent))
return nullptr;
- TheCFG.swap(FCFG);
+ Expression = Parent;
}
- 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;
+// Keep the destination in the source's lexical scope so that moving ownership
+// into an inner block cannot release a resource before the source leaves scope.
+static const Stmt *variableScope(const VarDecl *Variable, ASTContext &Context) {
+ // Lambda parameters can have both the call operator and the lambda itself
+ // as AST parents. Their lexical scope is nevertheless the callable's body.
+ if (isa<ParmVarDecl>(Variable)) {
+ const auto *Function =
+ dyn_cast_or_null<FunctionDecl>(Variable->getParentFunctionOrMethod());
+ return Function ? Function->getBody() : nullptr;
+ }
+ DynTypedNode Node = DynTypedNode::create(*Variable);
+ while (true) {
+ const DynTypedNodeList Parents = Context.getParents(Node);
+ if (Parents.size() != 1)
+ return nullptr;
+ Node = Parents[0];
+ if (const auto *Scope = Node.get<Stmt>();
+ Scope && isa<CompoundStmt, ForStmt, CXXForRangeStmt, IfStmt, SwitchStmt,
+ WhileStmt, CXXCatchStmt>(Scope))
+ return Scope;
+ if (const auto *Function = Node.get<FunctionDecl>())
+ return Function->getBody();
+ }
+}
- // The algorithm to look for a convertible move-assign operator is the
- // following: each node starts in the `Ready` state, with a number of
- // `RemainingSuccessors` equal to its number of successors.
- //
- // Starting from the exit node, we walk the CFG backward. Whenever
- // we meet a new block, we check if it either:
- // 1. touches the `AssignValue`, in which case we stop the search, and mark
- // each predecessor as not `Ready`. No predecessor walk.
- // 2. contains a convertible copy-assign operator, in which case we generate a
- // fix, and mark each predecessor as not Ready. No predecessor walk.
- // 3. does not interact with `AssignValue`, in which case we decrement the
- // `RemainingSuccessors` of each predecessor. And if it happens to turn to
- // 0 while still being `Ready`, we add it to the `WorkList`.
-
- struct BlockState {
- bool Ready;
- unsigned RemainingSuccessors;
- };
- llvm::DenseMap<const CFGBlock *, BlockState> CFGState;
- for (const auto *B : *TheCFG)
- CFGState.try_emplace(B, BlockState{true, B->succ_size()});
-
- const CFGBlock &TheExit = TheCFG->getExit();
- std::vector<const CFGBlock *> WorkList = {&TheExit};
+static bool cannotThrow(const FunctionDecl *Function) {
+ return Function->getType()->castAs<FunctionProtoType>()->canThrow() ==
+ CT_Cannot;
+}
- while (!WorkList.empty()) {
- const CFGBlock *B = WorkList.back();
- WorkList.pop_back();
- const BlockState &BS = CFGState.find(B)->second;
- if (!BS.Ready)
+// This deliberately handles only ordinary, unambiguously applicable special
+// members. General counterfactual overload resolution would require Sema.
+static const CXXMethodDecl *findMove(const CXXMethodDecl *Copy,
+ const Expr *Target = nullptr) {
+ const CXXRecordDecl *Record = Copy->getParent()->getDefinition();
+ if (!Record || Copy->isTrivial() || Record->isInvalidDecl())
+ return nullptr;
+ const CXXMethodDecl *Move = nullptr;
+ for (const CXXMethodDecl *Method : Record->methods()) {
+ const auto *Constructor = dyn_cast<CXXConstructorDecl>(Method);
+ if (Copy->isCopyAssignmentOperator()
+ ? !Method->isMoveAssignmentOperator()
+ : !Constructor || !Constructor->isMoveConstructor())
continue;
+ QualType Pointee = Method->getParamDecl(0)->getType()->getPointeeType();
+ if (Pointee.isNull() || Pointee.isConstQualified() ||
+ Pointee.isVolatileQualified())
+ continue;
+ if (Target &&
+ ((Target->getType().isConstQualified() && !Method->isConst()) ||
+ (Target->getType().isVolatileQualified() && !Method->isVolatile()) ||
+ (Method->getRefQualifier() == RQ_RValue && Target->isLValue()) ||
+ (Method->getRefQualifier() == RQ_LValue && !Target->isLValue())))
+ continue;
+ if (Move || Method->isDeleted() || Method->isVariadic() ||
+ Method->isExplicitObjectMemberFunction() ||
+ Method->getAccess() != AS_public ||
+ Method->getTrailingRequiresClause() || Method->isInvalidDecl())
+ return nullptr;
+ Move = Method;
+ }
+ return Move;
+}
- assert(BS.RemainingSuccessors == 0 &&
- "All successors have been processed.");
- bool ReferencesAssignedValue = false;
- for (const CFGElement &Elt : llvm::reverse(*B)) {
- if (Elt.getKind() != CFGElement::Kind::Statement)
- continue;
+static bool contains(const Stmt *Parent, const Stmt *Child) {
+ if (Parent == Child)
+ return true;
+ return Parent && llvm::any_of(Parent->children(), [&](const Stmt *S) {
+ return contains(S, Child);
+ });
+}
- const Stmt *EltStmt = Elt.castAs<CFGStmt>().getStmt();
- if (EltStmt == AssignExpr) {
- const StringRef AssignValueName = AssignValue->getDecl()->getName();
- diag(AssignValue->getBeginLoc(), "'%0' could be moved here")
- << AssignValueName
- << FixItHint::CreateReplacement(
- AssignValue->getLocation(),
- ("std::move(" + AssignValueName + ")").str());
- ReferencesAssignedValue = true;
- break;
- }
+// EH edges can put unordered operands into distinct CFG blocks. Check their
+// common expression, rather than trusting the CFG's chosen evaluation order.
+static bool unorderedWith(const Expr *Copy, const DeclRefExpr *Reference,
+ ASTContext &Context) {
+ const Expr *Expression = Copy;
+ while (true) {
+ const DynTypedNodeList Parents = Context.getParents(*Expression);
+ if (Parents.size() != 1)
+ return true;
+ const auto *Parent = Parents[0].get<Expr>();
+ if (!Parent)
+ return false;
+ if (const auto *Call = dyn_cast<CallExpr>(Parent)) {
+ if (const auto *Member = dyn_cast<CXXMemberCallExpr>(Call);
+ Member && contains(Member->getImplicitObjectArgument(), Reference))
+ return true;
+ for (const Expr *Arg : Call->arguments())
+ if (!contains(Arg, Copy) && contains(Arg, Reference))
+ return true;
+ } else if (const auto *Construction = dyn_cast<CXXConstructExpr>(Parent)) {
+ if (!Construction->isListInitialization())
+ for (const Expr *Arg : Construction->arguments())
+ if (!contains(Arg, Copy) && contains(Arg, Reference))
+ return true;
+ } else if (const auto *Binary = dyn_cast<BinaryOperator>(Parent)) {
+ if (Binary->getOpcode() != BO_Comma && Binary->getOpcode() != BO_LAnd &&
+ Binary->getOpcode() != BO_LOr &&
+ !(Binary->isAssignmentOp() && Context.getLangOpts().CPlusPlus17))
+ if ((contains(Binary->getLHS(), Copy) &&
+ contains(Binary->getRHS(), Reference)) ||
+ (contains(Binary->getRHS(), Copy) &&
+ contains(Binary->getLHS(), Reference)))
+ return true;
+ }
+ Expression = Parent;
+ }
+}
+
+struct UseStdMoveCheck::FunctionAnalysis
+ : RecursiveASTVisitor<UseStdMoveCheck::FunctionAnalysis> {
+ std::unique_ptr<CFG> Graph;
+ std::unique_ptr<utils::ExprSequence> Sequence;
+ std::unique_ptr<utils::StmtToBlockMap> BlockMap;
+ llvm::DenseMap<const VarDecl *, SmallVector<const DeclRefExpr *, 8>>
+ References;
+ llvm::DenseMap<const VarDecl *, const VarDecl *> Aliases;
+ SmallVector<const DeclStmt *, 8> Declarations;
+ SmallVector<const CallExpr *, 8> Calls;
+ llvm::DenseMap<const VarDecl *, bool> Escaped;
+
+ bool TraverseLambdaExpr(LambdaExpr *Lambda) {
+ // Capture initializers execute in this function; the body does not. In
+ // particular, a by-value capture's body refers to different storage.
+ for (Expr *Init : Lambda->capture_inits())
+ TraverseStmt(Init);
+ return true;
+ }
- // The reference is being referenced after the assignment.
- if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt,
- *Result.Context)
- .empty()) {
- ReferencesAssignedValue = true;
- break;
+ bool VisitDeclRefExpr(DeclRefExpr *Reference) {
+ if (const auto *Variable = dyn_cast<VarDecl>(Reference->getDecl()))
+ References[Variable].push_back(Reference);
+ return true;
+ }
+
+ bool VisitVarDecl(VarDecl *Variable) {
+ if (Variable->hasLocalStorage() &&
+ Variable->getType()->isLValueReferenceType() && Variable->hasInit())
+ if (const auto *Reference =
+ dyn_cast<DeclRefExpr>(Variable->getInit()->IgnoreParenImpCasts()))
+ if (const auto *Source = dyn_cast<VarDecl>(Reference->getDecl()))
+ Aliases.try_emplace(Variable, Source);
+ return true;
+ }
+
+ bool VisitDeclStmt(DeclStmt *Declaration) {
+ Declarations.push_back(Declaration);
+ return true;
+ }
+
+ bool VisitCallExpr(CallExpr *Call) {
+ Calls.push_back(Call);
+ return true;
+ }
+
+ const VarDecl *root(const VarDecl *Variable) const {
+ llvm::SmallPtrSet<const VarDecl *, 8> Seen;
+ while (Aliases.contains(Variable) && Seen.insert(Variable).second)
+ Variable = Aliases.lookup(Variable);
+ return Variable;
+ }
+
+ void collectAliases(ASTContext &Context) {
+ decltype(References) Merged;
+ for (const auto &[Declaration, Refs] : References)
+ for (const DeclRefExpr *Reference : Refs)
+ if (!ExprMutationAnalyzer::isUnevaluated(Reference, Context))
+ Merged[root(Declaration)].push_back(Reference);
+ Refere...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/228767
More information about the cfe-commits
mailing list