diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h --- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H #include "../ClangTidyCheck.h" +#include "../utils/ExprSequence.h" #include "../utils/IncludeInserter.h" #include @@ -31,8 +32,19 @@ void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: + /// Check a fixed parameter of a given function. Assumes that the type of the + /// parameter is suitable for pass-by-value. If `InitializersOnly` is false, + /// the parameters `TheCFG`, `StmtBlockMap` and `Sequence` must not be + /// `nullptr`. + void checkParameter(const FunctionDecl *, const ParmVarDecl *, + const CFG *TheCFG, + const utils::StmtToBlockMap *StmtBlockMap, + const utils::ExprSequence *Sequence, ASTContext &, + SourceManager &); + utils::IncludeInserter Inserter; const bool ValuesOnly; + const bool InitializersOnly; }; } // namespace modernize diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp --- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp @@ -7,6 +7,8 @@ //===----------------------------------------------------------------------===// #include "PassByValueCheck.h" +#include "../utils/Aliasing.h" +#include "../utils/ExprSequence.h" #include "clang/AST/ASTContext.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" @@ -23,89 +25,128 @@ namespace modernize { namespace { -/// Matches move-constructible classes. -/// -/// Given -/// \code -/// // POD types are trivially move constructible. -/// struct Foo { int a; }; -/// -/// struct Bar { -/// Bar(Bar &&) = deleted; -/// int a; -/// }; -/// \endcode -/// recordDecl(isMoveConstructible()) -/// matches "Foo". -AST_MATCHER(CXXRecordDecl, isMoveConstructible) { - for (const CXXConstructorDecl *Ctor : Node.ctors()) { - if (Ctor->isMoveConstructor() && !Ctor->isDeleted()) + +bool hasMoveConstructor(const CXXRecordDecl *Record) { + assert(Record); + + bool hasCopyCtor = false; + for (const CXXConstructorDecl *Ctor : Record->ctors()) { + if (Ctor->isMoveConstructor() && !Ctor->isDeleted()) { return true; + } + + if (Ctor->isCopyConstructor() && !Ctor->isDeleted()) { + hasCopyCtor = true; + } } + + if (hasCopyCtor && Record->needsImplicitMoveConstructor()) { + return true; + } + return false; } -} // namespace -static TypeMatcher notTemplateSpecConstRefType() { - return lValueReferenceType( - pointee(unless(elaboratedType(namesType(templateSpecializationType()))), - isConstQualified())); -} +bool hasMoveAssignmentOperator(const CXXRecordDecl *Record) { + assert(Record); + + bool hasCopyAssign = false; + for (const CXXMethodDecl *Method : Record->methods()) { + if (Method->isMoveAssignmentOperator() && !Method->isDeleted()) { + return true; + } + + if (Method->isCopyAssignmentOperator() && !Method->isDeleted()) { + hasCopyAssign = true; + } + } + + if (hasCopyAssign && Record->needsImplicitMoveAssignment()) { + return true; + } -static TypeMatcher nonConstValueType() { - return qualType(unless(anyOf(referenceType(), isConstQualified()))); + return false; } -/// Whether or not \p ParamDecl is used exactly one time in \p Ctor. +/// Tests whether the type \p T of a parameter is suitable for pass-by-value. /// -/// Checks both in the init-list and the body of the constructor. -static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor, - const ParmVarDecl *ParamDecl) { - /// \c clang::RecursiveASTVisitor that checks that the given - /// \c ParmVarDecl is used exactly one time. - /// - /// \see ExactlyOneUsageVisitor::hasExactlyOneUsageIn() - class ExactlyOneUsageVisitor - : public RecursiveASTVisitor { - friend class RecursiveASTVisitor; - - public: - ExactlyOneUsageVisitor(const ParmVarDecl *ParamDecl) - : ParamDecl(ParamDecl) {} - - /// Whether or not the parameter variable is referred only once in - /// the - /// given constructor. - bool hasExactlyOneUsageIn(const CXXConstructorDecl *Ctor) { - Count = 0; - TraverseDecl(const_cast(Ctor)); - return Count == 1; - } - - private: - /// Counts the number of references to a variable. - /// - /// Stops the AST traversal if more than one usage is found. - bool VisitDeclRefExpr(DeclRefExpr *D) { - if (const ParmVarDecl *To = dyn_cast(D->getDecl())) { - if (To == ParamDecl) { - ++Count; - if (Count > 1) { - // No need to look further, used more than once. - return false; - } - } - } - return true; +/// If \p ValuesOnly is true, we only consider value types. Otherwise, also +/// const reference types are allowed. The underlying type (after removing +/// references and qualifiers) must furthermore have a move constructor or a +/// move assignment operator, and it cannot be trivially copyable. +bool isCandidateType(QualType T, const bool ValuesOnly, ASTContext &Context) { + if (T.isNull()) { + return false; + } + + if (T.hasQualifiers()) { + return false; + } + + if (T.getNonReferenceType().isTriviallyCopyableType(Context)) { + return false; + } + + const CXXRecordDecl *R = T.getNonReferenceType()->getAsCXXRecordDecl(); + if (!R || (!hasMoveConstructor(R) && !hasMoveAssignmentOperator(R))) { + return false; + } + + if (const LValueReferenceType *LValueT = + dyn_cast(T)) { + if (ValuesOnly) { + return false; } - const ParmVarDecl *ParamDecl; - unsigned Count; - }; + const QualType PointeeType = LValueT->getPointeeType(); + + if (!PointeeType.getQualifiers().hasOnlyConst()) { + // We only consider references to const types. + return false; + } - return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor); + // TODO: Why do we do this at all, and why not for value types? + if (PointeeType->getAs()) { + return false; + } + + return true; + } + + if (T->isReferenceType()) { + return false; + } + + return true; } +/// A `RecursiveASTVisitor` to collect all usages of a variable. +/// +/// The usages are pushed to a vector that must be provided in the constructor. +class UsageFinder : public RecursiveASTVisitor { + friend class RecursiveASTVisitor; + +public: + UsageFinder(const VarDecl *Var, SmallVector *Usages) + : Var(Var), Usages(Usages) { + assert(Var); + assert(Usages); + } + +private: + bool VisitDeclRefExpr(DeclRefExpr *D) { + const ValueDecl *To = D->getDecl(); + if (To == Var) { + Usages->push_back(D); + } + + return true; + } + + const VarDecl *Var; + llvm::SmallVector *Usages; +}; + /// Returns true if the given constructor is part of a lvalue/rvalue reference /// pair, i.e. `Param` is of lvalue reference type, and there exists another /// constructor such that: @@ -129,8 +170,8 @@ /// /// A::A(const B& Param, int) /// A::A(B&& Param, int) -static bool hasRValueOverload(const CXXConstructorDecl *Ctor, - const ParmVarDecl *Param) { +bool hasRValueOverload(const CXXConstructorDecl *Ctor, + const ParmVarDecl *Param) { if (!Param->getType().getCanonicalType()->isLValueReferenceType()) { // The parameter is passed by value. return false; @@ -176,63 +217,382 @@ } /// Find all references to \p ParamDecl across all of the -/// redeclarations of \p Ctor. -static SmallVector -collectParamDecls(const CXXConstructorDecl *Ctor, - const ParmVarDecl *ParamDecl) { +/// redeclarations of \p Func. +SmallVector +collectParamDecls(const FunctionDecl *Func, const ParmVarDecl *ParamDecl) { SmallVector Results; unsigned ParamIdx = ParamDecl->getFunctionScopeIndex(); - for (const FunctionDecl *Redecl : Ctor->redecls()) + for (const FunctionDecl *Redecl : Func->redecls()) Results.push_back(Redecl->getParamDecl(ParamIdx)); return Results; } +/// Removes all nodes from a set of blocks in a CFG for which there exists a +/// non-empty path to some block that was initially in the set of blocks. +void retainFinalBlocks(llvm::SmallPtrSet &UsageBlocks, + const CFG &TheCFG) { + llvm::SmallPtrSet Visited; + llvm::SmallVector Open; + + for (const CFGBlock *Block : UsageBlocks) { + Open.push_back(Block); + Visited.insert(Block); + } + + while (!Open.empty()) { + const CFGBlock *B = Open.back(); + Open.pop_back(); + for (const CFGBlock *Pred : B->preds()) { + if (!Pred) { + // This happens if an apparent predecessor block has been proven to not + // ever transition to B. Happens e.g. with unreachable statements or + // `if (expr) { ... }` where `expr` can be proven to be always true or + // always false. + continue; + } + UsageBlocks.erase(Pred); + + if (!Visited.contains(Pred)) { + Open.push_back(Pred); + Visited.insert(Pred); + } + } + } +} + +/// Returns true if every path from the entry block to the exit block in +/// \p TheCFG contains a block in \p Blocks. +bool mustTraverseBlocks(llvm::SmallPtrSet &Blocks, + const CFG &TheCFG) { + assert(!Blocks.contains(&TheCFG.getEntry())); + assert(!Blocks.contains(&TheCFG.getExit())); + + if (&TheCFG.getEntry() == &TheCFG.getExit()) { + return false; + } + + llvm::SmallPtrSet Visited; + llvm::SmallVector Open; + + Visited.insert(&TheCFG.getEntry()); + Open.push_back(&TheCFG.getEntry()); + + while (!Open.empty()) { + const CFGBlock &B = *Open.back(); + Open.pop_back(); + + for (const CFGBlock *Succ : B.succs()) { + if (Succ == &TheCFG.getExit()) { + return false; + } + + if (!Succ || Blocks.contains(Succ) || Visited.contains(Succ)) { + continue; + } + + Open.push_back(Succ); + Visited.insert(Succ); + } + } + + return true; +} + +/// Computes the statement that is executed last in an array of statements in a +/// `CFGBlock`. +/// +/// All statements must be in the same `CFGBlock`, and an `ExprSequence` that +/// was constructed for the containing function must be provided. +const Stmt *finalStmtInBlock(llvm::ArrayRef Statements, + const utils::ExprSequence &Sequence) { + for (const Stmt *S : Statements) { + const auto PotentiallyAfterIt = + std::find_if(Statements.begin(), Statements.end(), [&](const Stmt *T) { + return T != S && Sequence.potentiallyAfter(S, T); + }); + if (PotentiallyAfterIt == Statements.end()) { + return S; + } + } + return nullptr; +} + +/// Returns the parent expression if it is not spelled in source. +const Expr *getUnspelledParent(const Expr *Expression, ASTContext &Context) { + assert(Expression); + DynTypedNodeList Parents = Context.getParents(*Expression); + if (Parents.size() != 1) { + return nullptr; + } + + const Expr *Parent = Parents[0].get(); + if (!Parent) { + return nullptr; + } + + // FIXME: This is horribly inefficient, as `IgnoreUnlessSpelledInSource` will + // recurse until it finds a node that is spelled in source. Instead, we + // should check whether `Parent` is a node that can be implicit and has the + // same source range as `Expression`. + if (Parent->IgnoreUnlessSpelledInSource() != + Expression->IgnoreUnlessSpelledInSource()) { + return nullptr; + } + + return Parent; +} + +/// Returns true if the given \p Expression is the argument of a `return` +/// statement. +bool hasReturnParent(const Expr *Expression, ASTContext &Context) { + assert(Expression); + const Expr *SpelledParent = Expression; + while (const Expr *P = getUnspelledParent(SpelledParent, Context)) { + SpelledParent = P; + } + assert(SpelledParent); + + DynTypedNodeList Parents = Context.getParents(*SpelledParent); + if (Parents.size() != 1) { + return false; + } + + return Parents[0].get(); +} + +/// A value that represents whether a manual `std::move` call is necessary or +/// the move happens automatically. +enum class MoveMode { + automatic, + manual, +}; + +/// Checks whether a copy can be avoided if a given \p Expression is moved. +/// +/// If so, returns whether a manual call to `std::move` is necessary or the +/// move happens automatically. +std::optional benefitsFromMoving(const Expr *Expression, + ASTContext &Context) { + assert(Expression); + DynTypedNodeList Parents = Context.getParents(*Expression); + if (Parents.size() != 1) { + return std::nullopt; + } + const DynTypedNode &Parent = Parents[0]; + + if (const auto *ImpCast = Parent.get()) { + return benefitsFromMoving(ImpCast, Context); + } + if (const auto *Paren = Parent.get()) { + return benefitsFromMoving(Paren, Context); + } + + if (const auto *CtorExpr = Parent.get()) { + const CXXConstructorDecl *Ctor = CtorExpr->getConstructor(); + + if (!Ctor || !Ctor->isCopyConstructor()) { + return std::nullopt; + } + + if (!Ctor->getParent() || !hasMoveConstructor(Ctor->getParent())) { + return std::nullopt; + } + + const bool MoveIsAutomatic = hasReturnParent(CtorExpr, Context); + + return MoveIsAutomatic ? MoveMode::automatic : MoveMode::manual; + } + + if (const auto *OpCall = Parent.get()) { + const FunctionDecl *Function = OpCall->getDirectCallee(); + if (!Function) { + return std::nullopt; + } + + const CXXMethodDecl *Method = dyn_cast(Function); + if (!Method || !Method->isCopyAssignmentOperator()) { + return std::nullopt; + } + + if (!Method->getParent() || + !hasMoveAssignmentOperator(Method->getParent())) { + return std::nullopt; + } + + return MoveMode::manual; + } + + return std::nullopt; +} + +/// Computes replacements necessary so that a given \p Function takes +/// \p ParamDecl by value. +/// +/// Returns `nullopt` if no such replacement can be found. +std::optional> +changeParameterToValueType(const FunctionDecl *Function, + const ParmVarDecl *ParamDecl, SourceManager &SM, + const LangOptions &LangOpts) { + assert(Function); + assert(ParamDecl); + llvm::SmallVector Result; + + if (!ParamDecl->getType()->isLValueReferenceType()) { + // No need to rewrite if the parameter is already passed by value. + return Result; + } + + for (const ParmVarDecl *ParmDecl : collectParamDecls(Function, ParamDecl)) { + TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); + ReferenceTypeLoc RefTL = ParamTL.getAs(); + if (RefTL.isNull()) { + // We cannot rewrite this instance. The type is probably hidden behind + // some `typedef`. Do not offer a fix-it in this case. + return std::nullopt; + } + + TypeLoc ValueTL = RefTL.getPointeeLoc(); + CharSourceRange TypeRange = CharSourceRange::getTokenRange( + ParmDecl->getBeginLoc(), ParamTL.getEndLoc()); + std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange( + ValueTL.getSourceRange()), + SM, LangOpts) + .str(); + ValueStr += ' '; + Result.push_back(FixItHint::CreateReplacement(TypeRange, ValueStr)); + } + + return Result; +} + +/// Computes a list of replacements so that a given \p Var is passed to +/// `std::move`. +/// +/// Returns `nullopt` if no such replacement can be found. +std::optional> +InsertManualMove(const DeclRefExpr *Var, SourceManager &SM, + const LangOptions &LangOpts) { + assert(Var); + + // The function `getEndLoc` does not work properly on `DeclRefExpr`; it + // returns the same value as `getLocation()`. + // As a workaround, we try to find the token corresponding to the variable + // name and use the token end location. + Optional VarToken = Lexer::findNextToken( + Var->getBeginLoc().getLocWithOffset(-1), SM, LangOpts); + if (!VarToken || VarToken->getLocation() != Var->getBeginLoc()) { + return std::nullopt; + } + + llvm::SmallVector Result; + Result.push_back( + FixItHint::CreateInsertion(VarToken->getLocation(), "std::move(")); + Result.push_back(FixItHint::CreateInsertion(VarToken->getEndLoc(), ")")); + return Result; +} + +/// Computes the final usage of a \p ParamDecl in the list of initializers of a +/// \p Constructor. +/// Returns `nullptr` if no final usage can be found. +const DeclRefExpr * +finalUsageInInitializers(const CXXConstructorDecl *Constructor, + const ParmVarDecl *ParamDecl) { + assert(Constructor); + assert(ParamDecl); + + /// FIXME: Currently does not consider the order in which initializers are + /// evaluated, so only works if there is exactly one usage among all + /// initializers. + + if (hasRValueOverload(Constructor, ParamDecl)) { + return nullptr; + } + + // Check if ParamDecl is used exactly once. + SmallVector Usages; + for (const CXXCtorInitializer *Init : Constructor->inits()) { + UsageFinder(ParamDecl, &Usages) + .TraverseConstructorInitializer(const_cast(Init)); + } + if (Usages.size() != 1) { + return nullptr; + } + + return Usages[0]; +} + +/// Computes an array of final usages of a variable in a function. +/// +/// A list `U` of usages is considered final if the following conditions are +/// met: +/// - No usage can occur after a usage in `U` with respect to control flow. +/// - Every control flow path through the containing function must go through +/// a (necessarily unique) usage in `U`. +/// +/// Returns an empty array if no such list exists. +SmallVector +finalUsagesInBody(ArrayRef Usages, const CFG &TheCFG, + const utils::StmtToBlockMap &StmtBlockMap, + const utils::ExprSequence &Sequence) { + SmallPtrSet UsageBlocks; + for (const DeclRefExpr *Usage : Usages) { + const CFGBlock *Block = StmtBlockMap.blockContainingStmt(Usage); + assert(Block); + UsageBlocks.insert(Block); + } + + // Remove all blocks that can transition to a usage of the variable. + retainFinalBlocks(UsageBlocks, TheCFG); + + // Return an empty result if not every control flow path through the function + // traverses via one of the final blocks in which the variable is used. + if (!mustTraverseBlocks(UsageBlocks, TheCFG)) { + return {}; + } + + // For each final blocks in which the variable is used, check and extract the + // last usage of the variable within the block. + SmallVector FinalUsages; + for (const CFGBlock *Block : UsageBlocks) { + // Filter the list of all `Usages` down to those usages in `Block`. + SmallVector BlockUsages; + for (const Stmt *Usage : Usages) { + if (StmtBlockMap.blockContainingStmt(Usage) == Block) { + BlockUsages.push_back(Usage); + } + } + + const Stmt *FinalUsage = finalStmtInBlock(BlockUsages, Sequence); + if (!FinalUsage) { + return {}; + } + FinalUsages.push_back(cast(FinalUsage)); + } + assert(FinalUsages.size() == UsageBlocks.size()); + + return FinalUsages; +} + +} // namespace + PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), Inserter(Options.getLocalOrGlobal("IncludeStyle", utils::IncludeSorter::IS_LLVM), areDiagsSelfContained()), - ValuesOnly(Options.get("ValuesOnly", false)) {} + ValuesOnly(Options.get("ValuesOnly", false)), + InitializersOnly(Options.get("InitializersOnly", false)) {} void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IncludeStyle", Inserter.getStyle()); Options.store(Opts, "ValuesOnly", ValuesOnly); + Options.store(Opts, "InitializersOnly", InitializersOnly); } void PassByValueCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - traverse( - TK_AsIs, - cxxConstructorDecl( - forEachConstructorInitializer( - cxxCtorInitializer( - unless(isBaseInitializer()), - // Clang builds a CXXConstructExpr only when it knows - // which constructor will be called. In dependent contexts - // a ParenListExpr is generated instead of a - // CXXConstructExpr, filtering out templates automatically - // for us. - withInitializer(cxxConstructExpr( - has(ignoringParenImpCasts(declRefExpr(to( - parmVarDecl( - hasType(qualType( - // Match only const-ref or a non-const - // value parameters. Rvalues, - // TemplateSpecializationValues and - // const-values shouldn't be modified. - ValuesOnly - ? nonConstValueType() - : anyOf(notTemplateSpecConstRefType(), - nonConstValueType())))) - .bind("Param"))))), - hasDeclaration(cxxConstructorDecl( - isCopyConstructor(), unless(isDeleted()), - hasDeclContext( - cxxRecordDecl(isMoveConstructible()))))))) - .bind("Initializer"))) - .bind("Ctor")), - this); + traverse(TK_AsIs, functionDecl(isDefinition()).bind("Function")), this); } void PassByValueCheck::registerPPCallbacks(const SourceManager &SM, @@ -242,67 +602,183 @@ } void PassByValueCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Ctor = Result.Nodes.getNodeAs("Ctor"); - const auto *ParamDecl = Result.Nodes.getNodeAs("Param"); - const auto *Initializer = - Result.Nodes.getNodeAs("Initializer"); - SourceManager &SM = *Result.SourceManager; - - // If the parameter is used or anything other than the copy, do not apply - // the changes. - if (!paramReferredExactlyOnce(Ctor, ParamDecl)) + const FunctionDecl *Function = + Result.Nodes.getNodeAs("Function"); + assert(Function); + + const CXXConstructorDecl *Ctor = dyn_cast(Function); + const CXXMethodDecl *Method = dyn_cast(Function); + + // Exit early if we are only supposed to look at constructor initializers. + if (InitializersOnly && !Ctor) { + return; + } + + // We ignore copy/move constructors/assignment operators. + if (Ctor && (Ctor->isCopyConstructor() || Ctor->isMoveConstructor())) { return; + } + if (Method && (Method->isCopyAssignmentOperator() || + Method->isMoveAssignmentOperator())) { + return; + } - // If the parameter is trivial to copy, don't move it. Moving a trivially - // copyable type will cause a problem with performance-move-const-arg - if (ParamDecl->getType().getNonReferenceType().isTriviallyCopyableType( - *Result.Context)) + Stmt *Body = Function->getBody(); + if (!Body) { return; + } + + SmallVector CandidateParms; + for (const ParmVarDecl *ParamDecl : Function->parameters()) { + // We ignore parameters which have rvalue overloads. + // TODO: Also for methods or even general functions? + if (Ctor && hasRValueOverload(Ctor, ParamDecl)) { + continue; + } + + // Ignore parameters which are referenced or pointed to by a local variable. + if (utils::hasPtrOrReferenceInFunc(Function, ParamDecl)) { + continue; + } + + if (isCandidateType(ParamDecl->getType(), /* ValuesOnly = */ ValuesOnly, + *Result.Context)) { + CandidateParms.push_back(ParamDecl); + } + } - // Do not trigger if we find a paired constructor with an rvalue. - if (hasRValueOverload(Ctor, ParamDecl)) + if (CandidateParms.empty()) { return; + } + std::unique_ptr TheCFG; + std::unique_ptr StmtBlockMap; + std::unique_ptr Sequence; + + if (!InitializersOnly) { + CFG::BuildOptions Options; + TheCFG = CFG::buildCFG(nullptr, Body, Result.Context, Options); + StmtBlockMap = + std::make_unique(TheCFG.get(), Result.Context); + Sequence = std::make_unique(TheCFG.get(), Body, + Result.Context); + } + + for (const ParmVarDecl *ParamDecl : CandidateParms) { + checkParameter(Function, ParamDecl, TheCFG.get(), StmtBlockMap.get(), + Sequence.get(), *Result.Context, *Result.SourceManager); + } +} + +void PassByValueCheck::checkParameter(const FunctionDecl *Function, + const ParmVarDecl *ParamDecl, + const CFG *TheCFG, + const utils::StmtToBlockMap *StmtBlockMap, + const utils::ExprSequence *Sequence, + ASTContext &Context, SourceManager &SM) { + assert(Function); + assert(ParamDecl); + + Stmt *Body = Function->getBody(); + assert(Body); + + const auto *Ctor = dyn_cast(Function); + + SmallVector BodyUsages; + UsageFinder(ParamDecl, &BodyUsages).TraverseStmt(Body); + + SmallVector FinalUsages; + if (BodyUsages.empty()) { + // The variable is never used in the body of the function. If the function + // is a constructor, we still consider initializers. + if (!Ctor) { + return; + } + const DeclRefExpr *FinalUsage = finalUsageInInitializers(Ctor, ParamDecl); + if (!FinalUsage) { + return; + } + FinalUsages = {FinalUsage}; + } else { + if (InitializersOnly) { + return; + } + + assert(TheCFG); + assert(StmtBlockMap); + assert(Sequence); + FinalUsages = + finalUsagesInBody(BodyUsages, *TheCFG, *StmtBlockMap, *Sequence); + if (FinalUsages.empty()) { + return; + } + } + assert(!FinalUsages.empty()); + + // There is a set of `FinalUsages` of the variable so we could potentially + // move. Check whether moving final usages would prevent a copy in each + // instance. + SmallVector MoveModes; + for (const DeclRefExpr *FinalUsage : FinalUsages) { + std::optional MM = benefitsFromMoving(FinalUsage, Context); + if (!MM) { + return; + } + MoveModes.push_back(*MM); + } + assert(MoveModes.size() == FinalUsages.size()); + + // Moving would be benefitial for each final usage. Emit a diagnostic, and + // try to emit an automatic. An automatic fix consists of several + // replacements. Since we don't want to emit partial fixes, we first compute + // all fixes (which might fail) and then emit them all together. auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move"); - // If we received a `const&` type, we need to rewrite the function - // declarations. - if (ParamDecl->getType()->isLValueReferenceType()) { - // Check if we can succesfully rewrite all declarations of the constructor. - for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) { - TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); - ReferenceTypeLoc RefTL = ParamTL.getAs(); - if (RefTL.isNull()) { - // We cannot rewrite this instance. The type is probably hidden behind - // some `typedef`. Do not offer a fix-it in this case. + std::optional> ParameterChange = + changeParameterToValueType(Function, ParamDecl, SM, getLangOpts()); + if (!ParameterChange) { + return; + } + + llvm::SmallVector ManualMoveInsertions; + for (size_t i = 0; i != FinalUsages.size(); ++i) { + const DeclRefExpr *FinalUsage = FinalUsages[i]; + const MoveMode MM = MoveModes[i]; + + switch (MM) { + case MoveMode::automatic: + break; + case MoveMode::manual: { + std::optional> Insertions = + InsertManualMove(FinalUsage, SM, getLangOpts()); + if (!Insertions) { return; } + for (auto &hint : *Insertions) { + ManualMoveInsertions.push_back(std::move(hint)); + } + break; + } } - // Rewrite all declarations. - for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) { - TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); - ReferenceTypeLoc RefTL = ParamTL.getAs(); - - TypeLoc ValueTL = RefTL.getPointeeLoc(); - CharSourceRange TypeRange = CharSourceRange::getTokenRange( - ParmDecl->getBeginLoc(), ParamTL.getEndLoc()); - std::string ValueStr = - Lexer::getSourceText( - CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM, - getLangOpts()) - .str(); - ValueStr += ' '; - Diag << FixItHint::CreateReplacement(TypeRange, ValueStr); - } - } - - // Use std::move in the initialization list. - Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")") - << FixItHint::CreateInsertion( - Initializer->getLParenLoc().getLocWithOffset(1), "std::move(") - << Inserter.createIncludeInsertion( - Result.SourceManager->getFileID(Initializer->getSourceLocation()), - ""); + } + + // At this point, nothing can go wrong anymore. + Diag << *ParameterChange << ManualMoveInsertions; + + // Emit `` includes if necessary. This cannot fail. + for (size_t i = 0; i != FinalUsages.size(); ++i) { + const DeclRefExpr *FinalUsage = FinalUsages[i]; + const MoveMode MM = MoveModes[i]; + + switch (MM) { + case MoveMode::automatic: + break; + case MoveMode::manual: + Diag << Inserter.createIncludeInsertion( + SM.getFileID(FinalUsage->getLocation()), ""); + break; + } + } } } // namespace modernize diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst @@ -23,12 +23,6 @@ foo(get_str()); // prvalue -> move construction } -.. note:: - - Currently, only constructors are transformed to make use of pass-by-value. - Contributions that handle other situations are welcome! - - Pass-by-value in constructors ----------------------------- @@ -164,3 +158,10 @@ When `true`, the check only warns about copied parameters that are already passed by value. Default is `false`. + +.. option:: InitializersOnly + + When `true`, the check only applies to arguments of constructors that are + used exactly once in a member or base class initializer. When `false, also + arguments of general functions and their usage in bodies are considered. + Default is `false`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp @@ -11,6 +11,8 @@ Movable() = default; Movable(const Movable &) {} Movable(Movable &&) {} + Movable& operator=(const Movable&) = default; + Movable& operator=(Movable&&) = default; }; struct NotMovable { @@ -19,12 +21,18 @@ NotMovable(NotMovable &&) = delete; int a, b, c; }; + +struct MovableConstructible { + MovableConstructible() = default; + MovableConstructible(Movable m) {} +}; + } struct A { - A(const Movable &M) : M(M) {} + A(const Movable &MM) : M(MM) {} // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move [modernize-pass-by-value] - // CHECK-FIXES: A(Movable M) : M(std::move(M)) {} + // CHECK-FIXES: A(Movable MM) : M(std::move(MM)) {} Movable M; }; @@ -36,7 +44,88 @@ Movable M; }; -// Test that a parameter with more than one reference to it won't be changed. +// Tests a single usage in function with trivial control flow graph. +void straightLineManual(const Movable &m) { + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: void straightLineManual(Movable m) { + MovableConstructible mc{m}; + // CHECK-FIXES: MovableConstructible mc{std::move(m)}; +} + +// Tests a unique usage in a return statement. +Movable automatic(const Movable &m) { + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: Movable automatic(Movable m) { + return (m); + // CHECK-FIXES: return (m); +} + +// Tests a unique usage in a return statement with an implicit conversion. +MovableConstructible automaticWithImplicitConversion(const Movable &m) { + // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: MovableConstructible automaticWithImplicitConversion(Movable m) { + return m; + // CHECK-FIXES: return m; +} + +// Tests that we don't emit a diagnostic if the variable is pointed to by a +// local variable. +const Movable& pointed(const Movable& m) { + // CHECK-FIXES: const Movable& pointed(const Movable& m) { + const Movable* ptr = &m; + MovableConstructible mc{m}; + return *ptr; +} + +// Same as `pointed` but with a reference. +const Movable& referenced(const Movable& m) { + // CHECK-FIXES: const Movable& referenced(const Movable& m) { + const Movable& ref = m; + MovableConstructible mc{m}; + return ref; +} + +// Tests that we emit a diagnostic for more than one final usages due to `if` blocks. +MovableConstructible branchingIf(const Movable &m) { + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: MovableConstructible branchingIf(Movable m) { + volatile bool b = false; + if (b) { + if (!b) { + return m; + // CHECK-FIXES: return m; + } else { + MovableConstructible mc{m}; + // CHECK-FIXES: MovableConstructible mc{std::move(m)}; + return mc; + } + } + + return MovableConstructible{m}; + // CHECK-FIXES: return MovableConstructible{std::move(m)}; +} + +// Tests that we do not emit a diagnostic if the variable is not used in all paths. +MovableConstructible branchingIfNotFinal(const Movable &m) { + // CHECK-FIXES: MovableConstructible branchingIfNotFinal(const Movable &m) { + volatile bool b = false; + if (b) { + MovableConstructible mc{m}; + return mc; + } + + return MovableConstructible(); +} + +// Tests that we do not emit a diagnostic if the last usage happens in a loop. +void loop(const Movable &m) { + // CHECK-FIXES: void loop(const Movable &m) { + for (int i = 0; ++i; i != 10) { + MovableConstructible{m}; + } +} + +// Tests constructors with initializer lists and non-trivial bodies. struct C { // Tests extra-reference in body. C(const Movable &M) : M(M) { this->i = M.a; } @@ -45,6 +134,15 @@ // Tests extra-reference in init-list. C(const Movable &M, int) : M(M), i(M.a) {} // CHECK-FIXES: C(const Movable &M, int) : M(M), i(M.a) {} + + // Tests final usage in the body of a constructor with initializers. + C(const Movable &MM, float) : i(MM.a) { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: C(Movable MM, float) : i(MM.a) { + M = MM; + // CHECK-FIXES: M = std::move(MM); + } + Movable M; int i; }; @@ -186,8 +284,7 @@ Movable M; }; -// Test with multiples parameters where some need to be changed and some don't. -// need to. +// Test with multiple parameters where some need to be changed and some don't. struct Q { Q(const Movable &A, const Movable &B, const Movable &C, double D) : A(A), B(B), C(C), D(D) {}