[clang-tidy] Add check performance-lost-std-move

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

@@ -12,6 +12,7 @@ add_clang_library(clangTidyPerformanceModule STATIC
   InefficientAlgorithmCheck.cpp
   InefficientStringConcatenationCheck.cpp
   InefficientVectorOperationCheck.cpp
+  LostStdMoveCheck.cpp
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp
   NoAutomaticMoveCheck.cpp

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

@@ -0,0 +1,196 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "LostStdMoveCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+template <typename Node>
+static void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
+                               llvm::SmallPtrSet<const Node *, 16> &Nodes) {
+  for (const BoundNodes &Match : Matches)
+    Nodes.insert(Match.getNodeAs<Node>(ID));
+}
+
+static llvm::SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprsHonourLambda(const VarDecl &VarDecl, const Decl &Decl,
+                            ASTContext &Context) {
+  auto Matches = match(
+      decl(forEachDescendant(
+          declRefExpr(to(varDecl(equalsNode(&VarDecl))),
+                      unless(hasAncestor(lambdaExpr(hasAnyCapture(lambdaCapture(
+                          capturesVar(varDecl(equalsNode(&VarDecl)))))))))
+              .bind("declRef"))),
+      Decl, Context);
+  llvm::SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  return DeclRefs;
+}
+
+static llvm::SmallPtrSet<const VarDecl *, 16>
+allVarDeclsExprs(const VarDecl &VarDecl, const Decl &Decl,
+                 ASTContext &Context) {
+  auto Matches = match(
+      decl(forEachDescendant(declRefExpr(
+          to(varDecl(equalsNode(&VarDecl))),
+          hasParent(decl(
+              varDecl(hasType(qualType(referenceType()))).bind("varDecl"))),
+          unless(hasAncestor(lambdaExpr(hasAnyCapture(
+              lambdaCapture(capturesVar(varDecl(equalsNode(&VarDecl))))))))))),
+      Decl, Context);
+  llvm::SmallPtrSet<const class VarDecl *, 16> VarDecls;
+  extractNodesByIdTo(Matches, "varDecl", VarDecls);
+  return VarDecls;
+}
+
+static const Expr *
+getLastVarUsage(const llvm::SmallPtrSet<const DeclRefExpr *, 16> &Exprs) {
+  const Expr *LastExpr = nullptr;
+  for (const clang::DeclRefExpr *Expr : Exprs) {
+    if (!LastExpr)
+      LastExpr = Expr;
+
+    if (LastExpr->getBeginLoc() < Expr->getBeginLoc())
+      LastExpr = Expr;
+  }
+
+  return LastExpr;
+}
+
+namespace {
+
+AST_MATCHER(CXXRecordDecl, hasTrivialMoveConstructor) {
+  return Node.hasDefinition() && Node.hasTrivialMoveConstructor();
+}
+
+AST_MATCHER_P(Expr, ignoreParens, ast_matchers::internal::Matcher<Expr>,
+              InnerMatcher) {
+  return InnerMatcher.matches(*Node.IgnoreParens(), Finder, Builder);
+}
+
+} // namespace
+
+void LostStdMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "StrictMode", StrictMode);
+}
+
+LostStdMoveCheck::LostStdMoveCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StrictMode(Options.get("StrictMode", false)) {}
+
+void LostStdMoveCheck::registerMatchers(MatchFinder *Finder) {
+  auto ReturnParent =
+      hasParent(expr(hasParent(cxxConstructExpr(hasParent(returnStmt())))));
+
+  auto OutermostExpr = expr(unless(hasParent(expr())));
+  auto LeafStatement = stmt(OutermostExpr);
+
+  Finder->addMatcher(
+      cxxConstructExpr(has(expr(has(ignoreParens(
+          declRefExpr(
+              // not "return x;"
+              unless(ReturnParent),
+              unless(hasType(namedDecl(hasName("::std::string_view")))),
+              // non-trivial type
+              hasType(hasCanonicalType(hasDeclaration(cxxRecordDecl()))),
+              // non-trivial X(X&&)
+              unless(hasType(hasCanonicalType(
+                  hasDeclaration(cxxRecordDecl(hasTrivialMoveConstructor()))))),
+              // Not in a cycle
+              unless(hasAncestor(forStmt())), unless(hasAncestor(doStmt())),
+              unless(hasAncestor(whileStmt())),
+              // Not in a body of lambda
+              unless(hasAncestor(compoundStmt(hasAncestor(lambdaExpr())))),
+              // only non-X&
+              unless(hasDeclaration(
+                  varDecl(hasType(qualType(lValueReferenceType()))))),
+              hasAncestor(LeafStatement.bind("leaf_statement")),
+              hasDeclaration(varDecl(hasAncestor(functionDecl().bind("func")))
+                                 .bind("decl")))
+              .bind("use")))))),
+      this);
+}
+
+void LostStdMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("decl");
+  const auto *MatchedFunc = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  const auto *MatchedUse = Result.Nodes.getNodeAs<Expr>("use");
+  const auto *MatchedLeafStatement =
+      Result.Nodes.getNodeAs<Stmt>("leaf_statement");
+
+  if (!MatchedDecl->hasLocalStorage()) {
+    // global or static variable
+    return;
+  }
+
+  if (!StrictMode) {
+    llvm::SmallPtrSet<const VarDecl *, 16> AllVarDecls =
+        allVarDeclsExprs(*MatchedDecl, *MatchedFunc, *Result.Context);
+    if (!AllVarDecls.empty()) {
+      // x is referenced by local var, it may outlive the "use"
+      return;
+    }
+  }
+
+  llvm::SmallPtrSet<const DeclRefExpr *, 16> AllReferences =
+      allDeclRefExprsHonourLambda(*MatchedDecl, *MatchedFunc, *Result.Context);
+  const Expr *LastUsage = getLastVarUsage(AllReferences);
+
+  if (LastUsage && LastUsage->getBeginLoc() > MatchedUse->getBeginLoc()) {
+    // "use" is not the last reference to x
+    return;
+  }
+
+  if (LastUsage &&
+      LastUsage->getSourceRange() != MatchedUse->getSourceRange()) {
+    return;
+  }
+
+  // Calculate X usage count in the statement
+  llvm::SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+
+  SmallVector<BoundNodes, 1> Matches = match(
+      findAll(
+          declRefExpr(to(varDecl(equalsNode(MatchedDecl))),
+                      unless(hasAncestor(lambdaExpr(hasAnyCapture(lambdaCapture(
+                          capturesVar(varDecl(equalsNode(MatchedDecl))))))))
+
+                          )
+              .bind("ref")),
+      *MatchedLeafStatement, *Result.Context);
+  extractNodesByIdTo(Matches, "ref", DeclRefs);
+  if (DeclRefs.size() > 1) {
+    // Unspecified order of evaluation, e.g. f(x, x)
+    return;
+  }
+
+  const SourceManager &Source = Result.Context->getSourceManager();
+  const auto Range =
+      CharSourceRange::getTokenRange(MatchedUse->getSourceRange());
+  const StringRef NeedleExprCode =
+      Lexer::getSourceText(Range, Source, Result.Context->getLangOpts());
+
+  if (NeedleExprCode == "=") {
+    diag(MatchedUse->getBeginLoc(), "could be std::move()")
+        << FixItHint::CreateInsertion(MatchedUse->getBeginLoc(),
+                                      (MatchedDecl->getName() +
+                                       " = std::move(" +
+                                       MatchedDecl->getName() + "),")
+                                          .str());
+  } else {
+    diag(MatchedUse->getBeginLoc(), "could be std::move()")
+        << FixItHint::CreateReplacement(
+               Range, ("std::move(" + NeedleExprCode + ")").str());
+  }
+}
+
+} // namespace clang::tidy::performance

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

@@ -0,0 +1,38 @@
+//===----------------------------------------------------------------------===//
+//
+// 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_LOSTSTDMOVECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_LOSTSTDMOVECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::performance {
+
+/// Warns if copy constructor is used instead of std::move() and suggests a fix.
+/// It honours cycles, lambdas, and unspecified call order in compound
+/// expressions.
+///
+/// For the user-facing documentation see:
+/// https://clang.llvm.org/extra/clang-tidy/checks/performance/lost-std-move.html
+class LostStdMoveCheck : public ClangTidyCheck {
+public:
+  LostStdMoveCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+
+private:
+  const bool StrictMode;
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_LOSTSTDMOVECHECK_H

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

@@ -17,6 +17,7 @@
 #include "InefficientAlgorithmCheck.h"
 #include "InefficientStringConcatenationCheck.h"
 #include "InefficientVectorOperationCheck.h"
+#include "LostStdMoveCheck.h"
 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoAutomaticMoveCheck.h"
@@ -49,6 +50,7 @@ class PerformanceModule : public ClangTidyModule {
         "performance-inefficient-string-concatenation");
     CheckFactories.registerCheck<InefficientVectorOperationCheck>(
         "performance-inefficient-vector-operation");
+    CheckFactories.registerCheck<LostStdMoveCheck>("performance-lost-std-move");
     CheckFactories.registerCheck<MoveConstArgCheck>(
         "performance-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -203,6 +203,11 @@ New checks
   Finds virtual function overrides with different visibility than the function
   in the base class.
 
+- New :doc:`performance-lost-std-move
+  <clang-tidy/checks/performance/lost-std-move>` check.
+
+  Warns if copy constructor is used instead of ``std::move()`` and suggests a fix.
+
 - New :doc:`readability-redundant-parentheses
   <clang-tidy/checks/readability/redundant-parentheses>` check.
 

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

@@ -350,6 +350,7 @@ Clang-Tidy Checks
    :doc:`performance-inefficient-algorithm <performance/inefficient-algorithm>`, "Yes"
    :doc:`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>`,
    :doc:`performance-inefficient-vector-operation <performance/inefficient-vector-operation>`, "Yes"
+   :doc:`performance-lost-std-move <performance/lost-std-move>`, "Yes"
    :doc:`performance-move-const-arg <performance/move-const-arg>`, "Yes"
    :doc:`performance-move-constructor-init <performance/move-constructor-init>`,
    :doc:`performance-no-automatic-move <performance/no-automatic-move>`,

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

@@ -0,0 +1,48 @@
+.. title:: clang-tidy - performance-lost-std-move
+
+performance-lost-std-move
+=========================
+
+Warns if copy constructor is used instead of ``std::move()`` and suggests a fix.
+It honours cycles, lambdas, and unspecified call order in compound expressions.
+
+.. code-block:: c++
+
+   void f(X);
+
+   void g(X x) {
+     f(x);  // warning: Could be std::move() [performance-lost-std-move]
+   }
+
+It finds the last local variable usage, and if it is a copy, emits a warning.
+The check is based on pure AST matching and doesn't take into account any
+data flow information. Thus, it doesn't catch assign-after-copy cases.
+
+Also it does notice variable references "behind the scenes":
+
+.. code-block:: c++
+
+   void f(X);
+
+   void g(X x) {
+     auto &y = x;
+     f(x);  // does not emit a warning
+     y.f();  // because x is still used via y
+   }
+
+If you want to ignore assigns to reference variables, set :option:`StrictMode`
+to `true`.
+
+
+Options
+-------
+
+.. option:: StrictMode
+
+   A variable ``X`` can be referenced by another variable ``R``. In this case
+   the last variable usage might be not from ``X``, but from ``R``. It is quite
+   difficult to find in a large function, so if the plugin sees some ``R``
+   references ``X``, then it will not emit a warning for ``X`` not to provoke
+   false positive. If you're sure that such references don't extend ``X``
+   lifetime and ready to handle possible false positives, then set `StrictMode`
+   to `true`. Defaults to `false`.

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

@@ -0,0 +1,30 @@
+// RUN: %check_clang_tidy %s performance-lost-std-move %t -- -config='{CheckOptions: {performance-lost-std-move.StrictMode: true}}'
+
+namespace std {
+
+template<typename T>
+class shared_ptr {
+public:
+  T& operator*() { return reinterpret_cast<T&>(*this); }
+  shared_ptr() {}
+  shared_ptr(const shared_ptr<T>&) {}
+};
+
+template<typename T>
+T&& move(T&)
+{
+}
+
+} // namespace std
+
+int f(std::shared_ptr<int>);
+
+void f_copy_after_ref()
+{
+  std::shared_ptr<int> ptr;
+  auto& ref = ptr;
+  f(ptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: could be std::move() [performance-lost-std-move]
+  // CHECK-FIXES: f(std::move(ptr));
+  *ref = 1;
+}