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