[clang-tidy] New performance linter: performance-use-std-move (#179467)

This linter suggests calls to ``std::move`` when a costly copy would
happen otherwise. It does not try to suggest ``std::move`` when they are
valid but obviously not profitable (e.g. for trivially movable types)

This is a very simple version that only considers terminating basic
blocks. Further work will extend the approach through the control flow
graph.

It has already been used successfully on llvm/lib to submit bugs
#178174,
 #178169, #178176, #178172, #178175, #178180, #178178, #178177, #178179,
 #178173 and #178167, and on the firefox codebase to submit most of the
dependencies of bug https://bugzilla.mozilla.org/show_bug.cgi?id=2012658

clang-tools-extra/clang-tidy/performance/CMakeLists.txt

@@ -26,6 +26,7 @@ add_clang_library(clangTidyPerformanceModule STATIC
   TypePromotionInMathFnCheck.cpp
   UnnecessaryCopyInitializationCheck.cpp
   UnnecessaryValueParamCheck.cpp
+  UseStdMoveCheck.cpp
 
   LINK_LIBS
   clangTidy

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

@@ -28,6 +28,7 @@
 #include "TypePromotionInMathFnCheck.h"
 #include "UnnecessaryCopyInitializationCheck.h"
 #include "UnnecessaryValueParamCheck.h"
+#include "UseStdMoveCheck.h"
 
 namespace clang::tidy {
 namespace performance {
@@ -73,6 +74,7 @@ public:
         "performance-unnecessary-copy-initialization");
     CheckFactories.registerCheck<UnnecessaryValueParamCheck>(
         "performance-unnecessary-value-param");
+    CheckFactories.registerCheck<UseStdMoveCheck>("performance-use-std-move");
   }
 };
 

clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.cpp

@@ -0,0 +1,122 @@
+//===--- UseStdMoveCheck.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 "UseStdMoveCheck.h"
+
+#include "../utils/DeclRefExprUtils.h"
+
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+namespace {
+AST_MATCHER(CXXRecordDecl, hasNonTrivialMoveAssignment) {
+  return Node.hasNonTrivialMoveAssignment();
+}
+
+AST_MATCHER(QualType, isLValueReferenceType) {
+  return Node->isLValueReferenceType();
+}
+
+AST_MATCHER(DeclRefExpr, refersToEnclosingVariableOrCapture) {
+  return Node.refersToEnclosingVariableOrCapture();
+}
+
+AST_MATCHER(CXXOperatorCallExpr, isCopyAssignmentOperator) {
+  if (const auto *MD = dyn_cast_or_null<CXXMethodDecl>(Node.getDirectCallee()))
+    return MD->isCopyAssignmentOperator();
+  return false;
+}
+
+// Ignore nodes inside macros.
+AST_POLYMORPHIC_MATCHER(isInMacro,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(Stmt, Decl)) {
+  return Node.getBeginLoc().isMacroID() || Node.getEndLoc().isMacroID();
+}
+} // namespace
+
+using utils::decl_ref_expr::allDeclRefExprs;
+
+void UseStdMoveCheck::registerMatchers(MatchFinder *Finder) {
+  auto AssignOperatorExpr =
+      cxxOperatorCallExpr(
+          isCopyAssignmentOperator(),
+          hasArgument(0, hasType(cxxRecordDecl(hasNonTrivialMoveAssignment()))),
+          hasArgument(
+              1, declRefExpr(
+                     to(varDecl(
+                         hasLocalStorage(),
+                         hasType(qualType(unless(anyOf(
+                             isLValueReferenceType(),
+                             isConstQualified() // Not valid to move const obj.
+                             )))))),
+                     unless(refersToEnclosingVariableOrCapture()))
+                     .bind("assign-value")),
+          forCallable(functionDecl().bind("within-func")), unless(isInMacro()))
+          .bind("assign");
+  Finder->addMatcher(AssignOperatorExpr, this);
+}
+
+const CFG *UseStdMoveCheck::getCFG(const FunctionDecl *FD,
+                                   ASTContext *Context) {
+  std::unique_ptr<CFG> &TheCFG = CFGCache[FD];
+  if (!TheCFG) {
+    const CFG::BuildOptions Options;
+    std::unique_ptr<CFG> FCFG =
+        CFG::buildCFG(nullptr, FD->getBody(), Context, Options);
+    if (!FCFG)
+      return nullptr;
+    TheCFG.swap(FCFG);
+  }
+  return TheCFG.get();
+}
+
+void UseStdMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *AssignExpr = Result.Nodes.getNodeAs<Expr>("assign");
+  const auto *AssignValue = Result.Nodes.getNodeAs<DeclRefExpr>("assign-value");
+  const auto *WithinFunctionDecl =
+      Result.Nodes.getNodeAs<FunctionDecl>("within-func");
+
+  const CFG *TheCFG = getCFG(WithinFunctionDecl, Result.Context);
+  if (!TheCFG)
+    return;
+
+  // Walk the CFG bottom-up, starting with the exit node.
+  // TODO: traverse the whole CFG instead of only considering terminator
+  // nodes.
+  const CFGBlock &TheExit = TheCFG->getExit();
+  for (auto &Pred : TheExit.preds()) {
+    if (!Pred.isReachable())
+      continue;
+    for (const CFGElement &Elt : llvm::reverse(*Pred)) {
+      if (Elt.getKind() != CFGElement::Kind::Statement)
+        continue;
+
+      const Stmt *EltStmt = Elt.castAs<CFGStmt>().getStmt();
+      if (EltStmt == AssignExpr) {
+        diag(AssignValue->getBeginLoc(), "'%0' could be moved here")
+            << AssignValue->getDecl()->getName();
+        break;
+      }
+      // The reference is being referenced after the assignment, bail out.
+      if (!allDeclRefExprs(*cast<VarDecl>(AssignValue->getDecl()), *EltStmt,
+                           *Result.Context)
+               .empty())
+        break;
+    }
+  }
+}
+
+} // namespace clang::tidy::performance

clang-tools-extra/clang-tidy/performance/UseStdMoveCheck.h

@@ -0,0 +1,35 @@
+//===--- InefficientCopyAssign.h - 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+#include "clang/Analysis/CFG.h"
+
+namespace clang::tidy::performance {
+
+class UseStdMoveCheck : public ClangTidyCheck {
+public:
+  UseStdMoveCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  llvm::DenseMap<const FunctionDecl *, std::unique_ptr<CFG>> CFGCache;
+  const CFG *getCFG(const FunctionDecl *, ASTContext *);
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_USESTDMOVECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -142,6 +142,12 @@ New checks
   Finds and removes redundant conversions from ``std::[w|u8|u16|u32]string_view`` to
   ``std::[...]string`` in call expressions expecting ``std::[...]string_view``.
 
+- New :doc:`performance-use-std-move
+  <clang-tidy/checks/performance/use-std-move>` check.
+
+  Suggests insertion of ``std::move(...)`` to turn copy assignment operator
+  calls into move assignment ones, when deemed valid and profitable.
+
 - New :doc:`readability-trailing-comma
   <clang-tidy/checks/readability/trailing-comma>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -369,6 +369,7 @@ Clang-Tidy Checks
    :doc:`performance-type-promotion-in-math-fn <performance/type-promotion-in-math-fn>`, "Yes"
    :doc:`performance-unnecessary-copy-initialization <performance/unnecessary-copy-initialization>`, "Yes"
    :doc:`performance-unnecessary-value-param <performance/unnecessary-value-param>`, "Yes"
+   :doc:`performance-use-std-move <performance/use-std-move>`,
    :doc:`portability-avoid-pragma-once <portability/avoid-pragma-once>`,
    :doc:`portability-restrict-system-includes <portability/restrict-system-includes>`, "Yes"
    :doc:`portability-simd-intrinsics <portability/simd-intrinsics>`,

clang-tools-extra/docs/clang-tidy/checks/performance/use-std-move.rst

@@ -0,0 +1,23 @@
+.. title:: clang-tidy - performance-use-std-move
+
+performance-use-std-move
+========================
+
+Suggests insertion of ``std::move(...)`` to turn copy assignment operator calls
+into move assignment ones, when deemed valid and profitable.
+
+.. code-block:: c++
+
+  void assign(std::vector<int>& out) {
+    std::vector<int> some = make_vector();
+    use_vector(some);
+    out = some;
+  }
+
+  // becomes
+
+  void assign(std::vector<int>& out) {
+    std::vector<int> some = make_vector();
+    use_vector(some);
+    out = std::move(some);
+  }

clang-tools-extra/test/clang-tidy/checkers/performance/use-std-move.cpp

@@ -0,0 +1,288 @@
+// RUN: %check_clang_tidy %s performance-use-std-move %t
+
+// Definitions used in the tests
+// -----------------------------
+
+namespace std {
+template<class T> struct remove_reference { typedef T type; };
+template<class T> struct remove_reference<T&> { typedef T type; };
+template<class T> struct remove_reference<T&&> { typedef T type; };
+template< class T >
+constexpr typename remove_reference<T>::type&& move( T&& t ) noexcept;
+}
+
+struct NonTrivialMoveAssign {
+  NonTrivialMoveAssign() = default;
+  NonTrivialMoveAssign(const NonTrivialMoveAssign&) = default;
+
+  NonTrivialMoveAssign& operator=(const NonTrivialMoveAssign&);
+  NonTrivialMoveAssign& operator=(NonTrivialMoveAssign&&);
+
+  NonTrivialMoveAssign& operator+=(const NonTrivialMoveAssign&);
+  NonTrivialMoveAssign& operator+=(NonTrivialMoveAssign&&);
+  void stuff();
+};
+
+struct TrivialMoveAssign {
+  TrivialMoveAssign& operator=(const TrivialMoveAssign&);
+  TrivialMoveAssign& operator=(TrivialMoveAssign&&) = default;
+};
+
+struct NoMoveAssign {
+  NoMoveAssign& operator=(const NoMoveAssign&);
+  NoMoveAssign& operator=(NoMoveAssign&&) = delete;
+};
+
+template<class T>
+void use(T&) {}
+
+// Check moving from various reference/pointer type
+// ------------------------------------------------
+
+void ConvertibleNonTrivialMoveAssign(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+
+void NonProfitableNonTrivialMoveAssignPointer(NonTrivialMoveAssign*& target, NonTrivialMoveAssign* source) {
+  // No message expected, moving is possible but non profitable for pointer.
+  target = source;
+}
+
+void ConvertibleNonTrivialMoveAssignFromLValue(NonTrivialMoveAssign& target, NonTrivialMoveAssign&& source) {
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+
+// Check moving already moved values
+// ---------------------------------
+
+void NonConvertibleNonTrivialMoveAssignAlreadyMoved(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // No message expected, it's already moved
+  target = std::move(source);
+}
+
+void NonConvertibleNonTrivialMoveAssignFromLValueAlreadyMoved(NonTrivialMoveAssign& target, NonTrivialMoveAssign&& source) {
+  // No message expected, it's already moved
+  target = std::move(source);
+}
+
+// Check moving within different context
+// -------------------------------------
+
+struct SomeRecord {
+void ConvertibleNonTrivialMoveAssignWithinMethod(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+};
+
+auto ConvertibleNonTrivialMoveAssignWithinLambda = [](NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+};
+
+void SomeFunction(NonTrivialMoveAssign source0, NonTrivialMoveAssign const &source1) {
+
+auto NonConvertibleNonTrivialMoveAssignWithinLambdaAsCaptureByRef = [&](NonTrivialMoveAssign& target) {
+  // No message expected, cannot move a non-local variable.
+  target = source0;
+  // No message expected, cannot move a non-local variable.
+  target = source1;
+};
+
+auto ConvertibleNonTrivialMoveAssignWithinLambdaAsCapture = [=](NonTrivialMoveAssign& target) {
+  // No message expected, cannot move a non-local variable.
+  target = source0;
+  // No message expected, cannot move a non-local variable.
+  target = source1;
+};
+
+}
+
+void ConvertibleNonTrivialMoveAssignShadowing(NonTrivialMoveAssign& target, NoMoveAssign source) {
+  {
+    NonTrivialMoveAssign source;
+    // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+    target = source;
+  }
+}
+
+void ConvertibleNonTrivialMoveAssignShadowed(NoMoveAssign& target, NonTrivialMoveAssign source) {
+  {
+    NoMoveAssign source;
+    // No message expected, `source' refers to a non-movable variable.
+    target = source;
+  }
+}
+
+#define ASSIGN(x, y) x = y
+void ConvertibleNonTrivialMoveAssignWithinMacro(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // No message expected, assignment within a macro.
+  ASSIGN(target, source);
+}
+
+template<class T>
+void ConvertibleNonTrivialMoveAssignWithTemplateSource(NonTrivialMoveAssign& target, T source) {
+  // No message expected, type of source cannot be matched against `target's type.
+  target = source;
+}
+
+template<class T>
+void ConvertibleNonTrivialMoveAssignWithTemplateTarget(T& target, NonTrivialMoveAssign source) {
+  // No message expected, type of target cannot be matched against `source's type.
+  target = source;
+}
+
+// Check moving from various storage
+// ---------------------------------
+
+void NonConvertibleNonTrivialMoveAssignFromLocal(NonTrivialMoveAssign& target) {
+  const NonTrivialMoveAssign source;
+  // No message, moving a const-qualified value is not valid.
+  target = source;
+}
+
+void NonConvertibleNonTrivialMoveAssignFromConst(NonTrivialMoveAssign& target) {
+  NonTrivialMoveAssign source;
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+
+void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) {
+  static NonTrivialMoveAssign source;
+  // No message, the lifetime of `source' does not end with the scope of the function.
+  target = source;
+}
+
+struct NonConvertibleNonTrivialMoveAssignFromMember {
+  NonTrivialMoveAssign source;
+  void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign& target) {
+    // No message, `source' is not a local variable.
+    target = source;
+  }
+};
+
+void NonConvertibleNonTrivialMoveAssignFromExtern(NonTrivialMoveAssign& target) {
+  extern NonTrivialMoveAssign source;
+  // No message, the lifetime of `source' does not end with the scope of the function.
+  target = source;
+}
+
+void NonConvertibleNonTrivialMoveAssignFromTLS(NonTrivialMoveAssign& target) {
+  thread_local NonTrivialMoveAssign source;
+  // No message, the lifetime of `source' does not end with the scope of the function.
+  target = source;
+}
+
+NonTrivialMoveAssign global_source;
+void NonConvertibleNonTrivialMoveAssignToGlobal(NonTrivialMoveAssign& target) {
+  // No message, the lifetime of `source' does not end with the scope of the function.
+  target = global_source;
+}
+
+
+// Check moving to various storage
+// -------------------------------
+
+void ConvertibleNonTrivialMoveAssignToStatic(NonTrivialMoveAssign source) {
+  static NonTrivialMoveAssign target;
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+
+struct ConvertibleNonTrivialMoveAssignToMember {
+  NonTrivialMoveAssign target;
+  void NonConvertibleNonTrivialMoveAssignFromStatic(NonTrivialMoveAssign source) {
+    // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+    target = source;
+  }
+};
+
+void ConvertibleNonTrivialMoveAssignToExtern(NonTrivialMoveAssign source) {
+  extern NonTrivialMoveAssign target;
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+
+void ConvertibleNonTrivialMoveAssignToTLS(NonTrivialMoveAssign source) {
+  thread_local NonTrivialMoveAssign target;
+  // CHECK-MESSAGES: [[@LINE+1]]:12: warning: 'source' could be moved here [performance-use-std-move]
+  target = source;
+}
+
+NonTrivialMoveAssign global_target;
+void ConvertibleNonTrivialMoveAssignToGlobal(NonTrivialMoveAssign source) {
+  // CHECK-MESSAGES: [[@LINE+1]]:19: warning: 'source' could be moved here [performance-use-std-move]
+  global_target = source;
+}
+
+void NonConvertibleNonTrivialMoveAssignRValue(NonTrivialMoveAssign& target, NonTrivialMoveAssign const& source) {
+  // No message expected, moving a reference is invalid there.
+  target = source;
+}
+
+void NonProfitableTrivialMoveAssign(TrivialMoveAssign& target, TrivialMoveAssign source) {
+  // No message expected, moving is possible but pedantic.
+  target = source;
+}
+
+// Check moving in presence of control flow or use
+// -----------------------------------------------
+
+void ConvertibleNonTrivialMoveAssignWithBranching(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  if(cond) {
+    // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+    target = source;
+  }
+}
+
+void NonConvertibleNonTrivialMoveAssignWithBranchingAndUse(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  if(cond) {
+    // No message expected, moving would make use invalid.
+    target = source;
+  }
+  use(source);
+}
+
+void ConvertibleNonTrivialMoveAssignBothBranches(bool cond, NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  if(cond) {
+    // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+    target = source;
+  }
+  else {
+    source.stuff();
+    // CHECK-MESSAGES: [[@LINE+1]]:14: warning: 'source' could be moved here [performance-use-std-move]
+    target = source;
+  }
+}
+
+void NonConvertibleNonTrivialMoveAssignWithExtraUse(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // No message expected, moving would make the call to `stuff' invalid.
+  target = source;
+  source.stuff();
+}
+
+void NonConvertibleNonTrivialMoveAssignInLoop(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // No message expected, moving would make the next iteration invalid.
+  for(int i = 0; i < 10; ++i)
+    target = source;
+}
+
+// Check moving for invalid / non profitable type or operation
+// -----------------------------------------------------------
+
+void NonConvertibleNonTrivialMoveUpdateAssign(NonTrivialMoveAssign& target, NonTrivialMoveAssign source) {
+  // No message currently expected, we only consider assignment.
+  target += source;
+}
+
+void NonProfitableTrivialTypeAssign(int& target, int source) {
+  // No message needed, it's correct to move but pedantic.
+  target = source;
+}
+
+void InvalidMoveAssign(NoMoveAssign& target, NoMoveAssign source) {
+  // No message expected, moving is deleted.
+  target = source;
+}