[clang-tidy] new performance-no-automatic-move check.

Summary: The check flags constructs that prevent automatic move of local variables.

Reviewers: aaron.ballman

Subscribers: mgorny, xazax.hun, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D70390

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

@@ -9,6 +9,7 @@ add_clang_library(clangTidyPerformanceModule
   InefficientVectorOperationCheck.cpp
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp
+  NoAutomaticMoveCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   PerformanceTidyModule.cpp
   TriviallyDestructibleCheck.cpp

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

@@ -0,0 +1,74 @@
+//===--- NoAutomaticMoveCheck.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 "NoAutomaticMoveCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+NoAutomaticMoveCheck::NoAutomaticMoveCheck(StringRef Name,
+                                           ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      AllowedTypes(
+          utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
+
+void NoAutomaticMoveCheck::registerMatchers(MatchFinder *Finder) {
+  // Automatic move exists only for c++11 onwards.
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  const auto ConstLocalVariable =
+      varDecl(hasLocalStorage(), unless(hasType(lValueReferenceType())),
+              hasType(qualType(
+                  isConstQualified(),
+                  hasCanonicalType(matchers::isExpensiveToCopy()),
+                  unless(hasDeclaration(namedDecl(
+                      matchers::matchesAnyListedName(AllowedTypes)))))))
+          .bind("vardecl");
+
+  // A matcher for a `DstT::DstT(const Src&)` where DstT also has a
+  // `DstT::DstT(Src&&)`.
+  const auto LValueRefCtor = cxxConstructorDecl(
+      hasParameter(0,
+                   hasType(lValueReferenceType(pointee(type().bind("SrcT"))))),
+      ofClass(cxxRecordDecl(hasMethod(cxxConstructorDecl(
+          hasParameter(0, hasType(rValueReferenceType(
+                              pointee(type(equalsBoundNode("SrcT")))))))))));
+
+  Finder->addMatcher(
+      returnStmt(
+          hasReturnValue(ignoringElidableConstructorCall(ignoringParenImpCasts(
+              cxxConstructExpr(hasDeclaration(LValueRefCtor),
+                               hasArgument(0, ignoringParenImpCasts(declRefExpr(
+                                                  to(ConstLocalVariable)))))
+                  .bind("ctor_call"))))),
+      this);
+}
+
+void NoAutomaticMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Var = Result.Nodes.getNodeAs<VarDecl>("vardecl");
+  const auto *CtorCall = Result.Nodes.getNodeAs<Expr>("ctor_call");
+  diag(CtorCall->getExprLoc(), "constness of '%0' prevents automatic move")
+      << Var->getName();
+}
+
+void NoAutomaticMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AllowedTypes",
+                utils::options::serializeStringList(AllowedTypes));
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang

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

@@ -0,0 +1,36 @@
+//===--- NoAutomaticMoveCheck.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_PERFORMANCE_NOAUTOMATICMOVECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOAUTOMATICMOVECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+/// Finds local variables that cannot be automatically moved due to constness.
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance-no-automatic-move.html
+class NoAutomaticMoveCheck : public ClangTidyCheck {
+public:
+  NoAutomaticMoveCheck(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;
+
+private:
+  const std::vector<std::string> AllowedTypes;
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_NOAUTOMATICMOVECHECK_H

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

@@ -17,6 +17,7 @@
 #include "InefficientVectorOperationCheck.h"
 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "NoAutomaticMoveCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "TriviallyDestructibleCheck.h"
 #include "TypePromotionInMathFnCheck.h"
@@ -46,6 +47,8 @@ public:
         "performance-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "performance-move-constructor-init");
+    CheckFactories.registerCheck<NoAutomaticMoveCheck>(
+        "performance-no-automatic-move");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "performance-noexcept-move-constructor");
     CheckFactories.registerCheck<TriviallyDestructibleCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -143,6 +143,11 @@ Improvements to clang-tidy
   Finds Objective-C implementations that implement ``-isEqual:`` without also
   appropriately implementing ``-hash``.
 
+- New :doc:`performance-no-automatic-move
+  <clang-tidy/checks/performance-no-automatic-move>` check.
+
+  Finds local variables that cannot be automatically moved due to constness.
+
 - New :doc:`performance-trivially-destructible
   <clang-tidy/checks/performance-trivially-destructible>` check.
 

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

@@ -344,6 +344,7 @@ Clang-Tidy Checks
    performance-inefficient-vector-operation
    performance-move-const-arg
    performance-move-constructor-init
+   performance-no-automatic-move
    performance-noexcept-move-constructor
    performance-trivially-destructible
    performance-type-promotion-in-math-fn

clang-tools-extra/docs/clang-tidy/checks/performance-no-automatic-move.rst

@@ -0,0 +1,53 @@
+.. title:: clang-tidy - performance-no-automatic-move
+
+performance-no-automatic-move
+=============================
+
+Finds local variables that cannot be automatically moved due to constness.
+
+Under
+`certain conditions <https://en.cppreference.com/w/cpp/language/return#automatic_move_from_local_variables_and_parameters>`_,
+local values are automatically moved out when returning from a function. A
+common mistake is to declare local ``lvalue`` variables ``const``, which
+prevents the move.
+
+Example `[1] <https://godbolt.org/z/x7SYYA>`_:
+
+.. code-block:: c++
+
+  StatusOr<std::vector<int>> Cool() {
+    std::vector<int> obj = ...;
+    return obj;  // calls StatusOr::StatusOr(std::vector<int>&&)
+  }
+  
+  StatusOr<std::vector<int>> NotCool() {
+    const std::vector<int> obj = ...;
+    return obj;  // calls `StatusOr::StatusOr(const std::vector<int>&)`
+  }
+
+The former version (``Cool``) should be preferred over the latter (``Uncool``)
+as it will avoid allocations and potentially large memory copies.
+
+Semantics
+---------
+
+In the example above, ``StatusOr::StatusOr(T&&)`` have the same semantics as
+long as the copy and move constructors for ``T`` have the same semantics. Note
+that there is no guarantee that ``S::S(T&&)`` and ``S::S(const T&)`` have the
+same semantics for any single ``S``, so we're not providing automated fixes for
+this check, and judgement should be exerted when making the suggested changes.
+
+-Wreturn-std-move
+-----------------
+
+Another case where the move cannot happen is the following:
+
+.. code-block:: c++
+
+  StatusOr<std::vector<int>> Uncool() {
+    std::vector<int>&& obj = ...;
+    return obj;  // calls `StatusOr::StatusOr(const std::vector<int>&)`
+  }
+
+In that case the fix is more consensual: just `return std::move(obj)`.
+This is handled by the `-Wreturn-std-move` warning.

clang-tools-extra/test/clang-tidy/checkers/performance-no-automatic-move.cpp

@@ -0,0 +1,92 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17,c++2a %s performance-no-automatic-move %t
+
+struct Obj {
+  Obj();
+  Obj(const Obj &);
+  Obj(Obj &&);
+  virtual ~Obj();
+};
+
+template <typename T>
+struct StatusOr {
+  StatusOr(const T &);
+  StatusOr(T &&);
+};
+
+struct NonTemplate {
+  NonTemplate(const Obj &);
+  NonTemplate(Obj &&);
+};
+
+template <typename T>
+T Make();
+
+StatusOr<Obj> PositiveStatusOrConstValue() {
+  const Obj obj = Make<Obj>();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+NonTemplate PositiveNonTemplateConstValue() {
+  const Obj obj = Make<Obj>();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+Obj PositiveSelfConstValue() {
+  const Obj obj = Make<Obj>();
+  return obj;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents automatic move [performance-no-automatic-move]
+}
+
+// FIXME: Ideally we would warn here too.
+NonTemplate PositiveNonTemplateLifetimeExtension() {
+  const Obj &obj = Make<Obj>();
+  return obj;
+}
+
+// FIXME: Ideally we would warn here too.
+StatusOr<Obj> PositiveStatusOrLifetimeExtension() {
+  const Obj &obj = Make<Obj>();
+  return obj;
+}
+
+// Negatives.
+
+StatusOr<Obj> Temporary() {
+  return Make<const Obj>();
+}
+
+StatusOr<Obj> ConstTemporary() {
+  return Make<const Obj>();
+}
+
+StatusOr<Obj> Nrvo() {
+  Obj obj = Make<Obj>();
+  return obj;
+}
+
+StatusOr<Obj> Ref() {
+  Obj &obj = Make<Obj &>();
+  return obj;
+}
+
+StatusOr<Obj> ConstRef() {
+  const Obj &obj = Make<Obj &>();
+  return obj;
+}
+
+const Obj global;
+
+StatusOr<Obj> Global() {
+  return global;
+}
+
+struct FromConstRefOnly {
+  FromConstRefOnly(const Obj &);
+};
+
+FromConstRefOnly FromConstRefOnly() {
+  const Obj obj = Make<Obj>();
+  return obj;
+}