Generalize clang-tidy modernize-pass-by-value

[mbid]

Summary

Previously, the clang-tidy modernize-pass-by-value check would only fire
for arguments of constructors that are used exactly once and in a member
or base class initializers. This commit generalizes the check to general
functions and usages in the body of the function.

We now consider the control-flow graph of the function. If every
control-flow path must transition through one final usage of a given
parameter, we fire the diagnostic. We additionally require that the
variable is not referenced or pointed to by a local variable in the
function. There are still some false-positives, however, because we do
not try to figure out whether a pointer to the local variable escapes
through other means, e.g. here:

struct HasStrPtr {
  const std::string* str;
};

void set_ptr(HasStrPtr& has_ptr, const std::string& str) {
  has_ptr.str = &str;
}

// Take argument by value.
void foo(std::string);

void foo(const std::string& str) {
  HasStrPtr obj;
  set_ptr(obj, str);
  foo(str);
  foo(*HasStrPtr.str);
}

The example looks at first sight like we can std::move in the call to
foo, but this would change the value pointed to by the pointer in
HasStrPtr, so that the semantics of the program change.

[mbid]

Hi Nathan,

Could you have a look at whether something like this looks reasonable? The docs of modernize-pass-by-value currently say that a patch that generalizes this check is welcome. I'd polish the docs and add some more test cases before merge if this looks reasonable overall.

• Martin

[ccotter]

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

11

I think you need to add clangAnalysis (for Analysis/CFG.h) to the link libraries in modernize/CMakeLists.txt.

[ccotter]

https://reviews.llvm.org/D137205 looks to be similar in part, as it applies a FixIt to a any last use of an object if the use a copy, including parameters and local objects.

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h

 //===--- PassByValueCheck.h - clang-tidy-------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H
 #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>
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 class PassByValueCheck : public ClangTidyCheck {
 public:
   PassByValueCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_PASS_BY_VALUE_H

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

 //===--- PassByValueCheck.cpp - clang-tidy---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "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"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 
 using namespace clang::ast_matchers;
 using namespace llvm;
 
 namespace clang {
 namespace tidy {
 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;
     }
 
-static TypeMatcher nonConstValueType() {
-  return qualType(unless(anyOf(referenceType(), isConstQualified())));
+    if (Method->isCopyAssignmentOperator() && !Method->isDeleted()) {
+      hasCopyAssign = true;
+    }
   }
 
-/// Whether or not \p ParamDecl is used exactly one time in \p Ctor.
-///
-/// 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>;
+  if (hasCopyAssign && Record->needsImplicitMoveAssignment()) {
+    return true;
+  }
+
+  return false;
+}
+
+/// Tests whether the type \p T of a parameter is suitable for pass-by-value.
+/// 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;
+  }
 
-  public:
-    ExactlyOneUsageVisitor(const ParmVarDecl *ParamDecl)
-        : ParamDecl(ParamDecl) {}
+  if (T.hasQualifiers()) {
+    return false;
+  }
 
-    /// 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;
-    }
+  if (T.getNonReferenceType().isTriviallyCopyableType(Context)) {
+    return false;
 
-  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;
-    }
+  const CXXRecordDecl *R = T.getNonReferenceType()->getAsCXXRecordDecl();
+  if (!R || (!hasMoveConstructor(R) && !hasMoveAssignmentOperator(R))) {
+
+  if (const LValueReferenceType *LValueT =
+          dyn_cast<const LValueReferenceType>(T)) {
+    if (ValuesOnly) {
+      return false;
+
+    const QualType PointeeType = LValueT->getPointeeType();
+
+    if (!PointeeType.getQualifiers().hasOnlyConst()) {
+      // We only consider references to const types.
+      return false;
+
+    // TODO: Why do we do this at all, and why not for value types?
+    if (PointeeType->getAs<TemplateSpecializationType>()) {
+      return false;
+    }
+
 
-    const ParmVarDecl *ParamDecl;
-    unsigned Count;
-  };
+  if (T->isReferenceType()) {
+    return false;
+  }
 
-  return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor);
+  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:
 ///  - it has the same number of parameters as `Ctor`.
 ///  - the parameter at the same index as `Param` is an rvalue reference
 ///    of the same pointee type
 ///  - all other parameters have the same type as the corresponding parameter in
 ///    `Ctor` or are rvalue references with the same pointee type.
 /// Examples:
 ///  A::A(const B& Param)
 ///  A::A(B&&)
 ///
 ///  A::A(const B& Param, const C&)
 ///  A::A(B&& Param, C&&)
 ///
 ///  A::A(const B&, const C& Param)
 ///  A::A(B&&, C&& Param)
 ///
 ///  A::A(const B&, const C& Param)
 ///  A::A(const B&, C&& Param)
 ///
 ///  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,
   if (!Param->getType().getCanonicalType()->isLValueReferenceType()) {
     // The parameter is passed by value.
     return false;
   }
   const int ParamIdx = Param->getFunctionScopeIndex();
   const CXXRecordDecl *Record = Ctor->getParent();
 
   // Check whether a ctor `C` forms a pair with `Ctor` under the aforementioned
for (const auto *Candidate : Record->ctors()) {
     if (IsRValueOverload(Candidate)) {
       return true;
     }
   }
   return false;
 }
 
 /// 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,
                                            Preprocessor *PP,
                                            Preprocessor *ModuleExpanderPP) {
   Inserter.registerPreprocessor(PP);
 }
 
 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;
+  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);
 
-  // If the parameter is used or anything other than the copy, do not apply
-  // the changes.
-  if (!paramReferredExactlyOnce(Ctor, ParamDecl))
+  // Exit early if we are only supposed to look at constructor initializers.
+  if (InitializersOnly && !Ctor) {
     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))
+  // We ignore copy/move constructors/assignment operators.
+  if (Ctor && (Ctor->isCopyConstructor() || Ctor->isMoveConstructor())) {
+    return;
+  }
+  if (Method && (Method->isCopyAssignmentOperator() ||
+                 Method->isMoveAssignmentOperator())) {
     return;
+  }
 
-  // Do not trigger if we find a paired constructor with an rvalue.
-  if (hasRValueOverload(Ctor, ParamDecl))
+  Stmt *Body = Function->getBody();
+  if (!Body) {
     return;
+  }
 
-  auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
+  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;
+    }
 
-  // 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.
-        return;
-      }
-    }
-    // Rewrite all declarations.
-    for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
-      TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
-      ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
+    // 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);
+    }
+  }
+
+  if (CandidateParms.empty()) {
+
+  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);
+  }
 
-      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);
+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");
 
-  // 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>");
+  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;
+    }
+    }
+  }
+
+  // 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
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst

.. code-block:: c++
     void foo(std::string s);
     std::string get_str();
 
     void f(const std::string &str) {
       foo(str);       // lvalue  -> copy construction
       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
 -----------------------------
 
 Replaces the uses of const-references constructor parameters that are copied
 into class fields. The parameter is then moved with `std::move()`.
 
 Since ``std::move()`` is a library function declared in `<utility>` it may be
 necessary to add this include. The check will add the include directive when
.. option:: IncludeStyle
 
    A string specifying which include-style is used, `llvm` or `google`. Default
    is `llvm`.
 
 .. option:: ValuesOnly
 
    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`.

clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp

 // RUN: %check_clang_tidy %s modernize-pass-by-value %t -- -- -fno-delayed-template-parsing
 
 namespace {
 // POD types are trivially move constructible.
 struct POD {
   int a, b, c;
 };
 
 struct Movable {
   int a, b, c;
   Movable() = default;
   Movable(const Movable &) {}
   Movable(Movable &&) {}
+  Movable& operator=(const Movable&) = default;
+  Movable& operator=(Movable&&) = default;
 };
 
 struct NotMovable {
   NotMovable() = default;
   NotMovable(const NotMovable &) = default;
   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;
 };
 
 // Test that we aren't modifying other things than a parameter.
 Movable GlobalObj;
 struct B {
   B(const Movable &M) : M(GlobalObj) {}
   // CHECK-FIXES: B(const Movable &M) : M(GlobalObj) {}
   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; }
   // CHECK-FIXES: C(const Movable &M) : M(M) { this->i = M.a; }
 
   // 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;
 };
 
 // Test that both declaration and definition are updated.
 struct D {
   D(const Movable &M);
   // CHECK-FIXES: D(Movable M);

 
 // Test with a const-value parameter.
 struct P {
   P(const Movable M) : M(M) {}
   // CHECK-FIXES: P(const Movable M) : M(M) {}
   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) {}
   // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: pass by value and use std::move
   // CHECK-MESSAGES: :[[@LINE-3]]:41: warning: pass by value and use std::move
   // CHECK-FIXES:      Q(const Movable &A, Movable B, Movable C, double D)
   // CHECK-FIXES:     : A(A), B(std::move(B)), C(std::move(C)), D(D) {}
   const Movable &A;