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 <memory>
@@ -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<ExactlyOneUsageVisitor> {
- friend class RecursiveASTVisitor<ExactlyOneUsageVisitor>;
-
- 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<CXXConstructorDecl *>(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<ParmVarDecl>(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<const LValueReferenceType>(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<TemplateSpecializationType>()) {
+ 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<UsageFinder> {
+ friend class RecursiveASTVisitor<UsageFinder>;
+
+public:
+ UsageFinder(const VarDecl *Var, SmallVector<const DeclRefExpr *> *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<const DeclRefExpr *> *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<const ParmVarDecl *, 2>
-collectParamDecls(const CXXConstructorDecl *Ctor,
- const ParmVarDecl *ParamDecl) {
+/// redeclarations of \p Func.
+SmallVector<const ParmVarDecl *, 2>
+collectParamDecls(const FunctionDecl *Func, const ParmVarDecl *ParamDecl) {
SmallVector<const ParmVarDecl *, 2> 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<const CFGBlock *, 4> &UsageBlocks,
+ const CFG &TheCFG) {
+ llvm::SmallPtrSet<const CFGBlock *, 10> Visited;
+ llvm::SmallVector<const CFGBlock *> 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<const CFGBlock *, 4> &Blocks,
+ const CFG &TheCFG) {
+ assert(!Blocks.contains(&TheCFG.getEntry()));
+ assert(!Blocks.contains(&TheCFG.getExit()));
+
+ if (&TheCFG.getEntry() == &TheCFG.getExit()) {
+ return false;
+ }
+
+ llvm::SmallPtrSet<const CFGBlock *, 10> Visited;
+ llvm::SmallVector<const CFGBlock *> 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<const Stmt *> 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<Expr>();
+ 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<ReturnStmt>();
+}
+
+/// 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<MoveMode> 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<const ImplicitCastExpr>()) {
+ return benefitsFromMoving(ImpCast, Context);
+ }
+ if (const auto *Paren = Parent.get<const ParenExpr>()) {
+ return benefitsFromMoving(Paren, Context);
+ }
+
+ if (const auto *CtorExpr = Parent.get<const CXXConstructExpr>()) {
+ 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 CXXOperatorCallExpr>()) {
+ const FunctionDecl *Function = OpCall->getDirectCallee();
+ if (!Function) {
+ return std::nullopt;
+ }
+
+ const CXXMethodDecl *Method = dyn_cast<const CXXMethodDecl>(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<llvm::SmallVector<FixItHint>>
+changeParameterToValueType(const FunctionDecl *Function,
+ const ParmVarDecl *ParamDecl, SourceManager &SM,
+ const LangOptions &LangOpts) {
+ assert(Function);
+ assert(ParamDecl);
+ llvm::SmallVector<FixItHint> 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<ReferenceTypeLoc>();
+ 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<llvm::SmallVector<FixItHint>>
+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<Token> VarToken = Lexer::findNextToken(
+ Var->getBeginLoc().getLocWithOffset(-1), SM, LangOpts);
+ if (!VarToken || VarToken->getLocation() != Var->getBeginLoc()) {
+ return std::nullopt;
+ }
+
+ llvm::SmallVector<FixItHint> 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<const DeclRefExpr *> Usages;
+ for (const CXXCtorInitializer *Init : Constructor->inits()) {
+ UsageFinder(ParamDecl, &Usages)
+ .TraverseConstructorInitializer(const_cast<CXXCtorInitializer *>(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<const DeclRefExpr *>
+finalUsagesInBody(ArrayRef<const DeclRefExpr *> Usages, const CFG &TheCFG,
+ const utils::StmtToBlockMap &StmtBlockMap,
+ const utils::ExprSequence &Sequence) {
+ SmallPtrSet<const CFGBlock *, 4> 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<const DeclRefExpr *> FinalUsages;
+ for (const CFGBlock *Block : UsageBlocks) {
+ // Filter the list of all `Usages` down to those usages in `Block`.
+ SmallVector<const Stmt *> 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<const DeclRefExpr>(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<CXXConstructorDecl>("Ctor");
- const auto *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>("Param");
- const auto *Initializer =
- Result.Nodes.getNodeAs<CXXCtorInitializer>("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<FunctionDecl>("Function");
+ assert(Function);
+
+ const CXXConstructorDecl *Ctor = dyn_cast<const CXXConstructorDecl>(Function);
+ const CXXMethodDecl *Method = dyn_cast<const CXXMethodDecl>(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<const ParmVarDecl *> 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<CFG> TheCFG;
+ std::unique_ptr<utils::StmtToBlockMap> StmtBlockMap;
+ std::unique_ptr<utils::ExprSequence> Sequence;
+
+ if (!InitializersOnly) {
+ CFG::BuildOptions Options;
+ TheCFG = CFG::buildCFG(nullptr, Body, Result.Context, Options);
+ StmtBlockMap =
+ std::make_unique<utils::StmtToBlockMap>(TheCFG.get(), Result.Context);
+ Sequence = std::make_unique<utils::ExprSequence>(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<const CXXConstructorDecl>(Function);
+
+ SmallVector<const DeclRefExpr *> BodyUsages;
+ UsageFinder(ParamDecl, &BodyUsages).TraverseStmt(Body);
+
+ SmallVector<const DeclRefExpr *> 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<MoveMode> MoveModes;
+ for (const DeclRefExpr *FinalUsage : FinalUsages) {
+ std::optional<MoveMode> 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<ReferenceTypeLoc>();
- 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<llvm::SmallVector<FixItHint>> ParameterChange =
+ changeParameterToValueType(Function, ParamDecl, SM, getLangOpts());
+ if (!ParameterChange) {
+ return;
+ }
+
+ llvm::SmallVector<FixItHint> 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<llvm::SmallVector<FixItHint>> 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<ReferenceTypeLoc>();
-
- 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()),
- "<utility>");
+ }
+
+ // At this point, nothing can go wrong anymore.
+ Diag << *ParameterChange << ManualMoveInsertions;
+
+ // Emit `<utility>` 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()), "<utility>");
+ 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) {}