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

[courbet]

Summary

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

[JonasToth]

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

48

While checking this example it seems clang already has a warning for that case?

https://godbolt.org/z/q5zzh-

What parts of this check will be more then the warnings already do?

56

Does this syntax work in rst files?

[courbet]

Thanks for the comments

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

48

I was not aware of that, thanks for pointing that out. I don't think the check does more than the warning in that case. TBH I have not seen instances of this while running the check on our codebase (I'm only looking at a sample of the mistakes though, there are too many hits to look at all of them). All mistakes I have seen are of the const kind.

The value we get from having this in the form of a check is more control over which types are allowed through the clang-tidy options.

56

Nope, this was migrated from our internal markdown syntax, thanks.

[courbet]

Fix markdown in doc.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

149

Please omit The check and synchronize with first sentence of documentation.

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

11

Please omit This check,

[courbet]

Thanks.

[courbet]

Address comments.

[lebedev.ri]

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

48

The const std::vector<int> f isn't diagnosed by the existing diag: https://godbolt.org/z/ZTQ3H6

[JonasToth]

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

48

so should we split this up in a diagnostic and a clang-tidy check? maybe it makes more sense to improve the warning further?

[lebedev.ri]

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

48

I'd +1 just enhancing the clang diagnostic.
The downside is that it does not allow blacklist, sure.

[courbet]

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

48

OK I'll work on improving the warning.

[courbet]

Actually, thinking more about this, adding this to the existing warning might not be a very consensual change. In the case of the warning:

S<T> f() {
  T&& t = ...;
  ...
  return t;
}

there is no argument against doing the std::move(): the code is just as clear and has better performance:

S<T> f() {
  T&& t = ...;
  ...
  return std::move(t);
}

In the case of a const variable, some people might value the safety from the const above the performance:

S<T> f() {
  const T t = ...;
  ...  // here be dragons
  return t;
}

And they would be unhappy if the -Wreturn-std-move started warning about this.

So I think there are two options here:

• Add a new warning, or
• Add it as a clang-tidy as in this change.

What do you think ?

[courbet]

Adding @Quuxplusone (warning author) for opinions.

[JonasToth]

IMHO these two should just not overlap. It makes sense, to have controversial or configurable stuff in clang-tidy. It should just be consistent with the warnings, as those are "always right" and clang-tidy can be opinionated/specialized.

[courbet]

IMHO these two should just not overlap. It makes sense, to have controversial or configurable stuff in clang-tidy. It should just be consistent with the warnings, as those are "always right" and clang-tidy can be opinionated/specialized.

So to make sure I understand you're advocating for keeping the const version in the clang-tidy check but removing the && detection from this check and let the warning deal with that ?

[JonasToth]

In D70390#1751159, @courbet wrote:

IMHO these two should just not overlap. It makes sense, to have controversial or configurable stuff in clang-tidy. It should just be consistent with the warnings, as those are "always right" and clang-tidy can be opinionated/specialized.

So to make sure I understand you're advocating for keeping the const version in the clang-tidy check but removing the && detection from this check and let the warning deal with that ?

Yes.

[• Quuxplusone]

Hi, I'm the main original author (although I would not claim "current maintainer") of the Clang warning.
Clang doesn't warn about const vector<int> v; return v; because in that case, adding std::move to the return would not help anything. I had not considered that we might suggest a "fixit" of "Change the declared type of this variable from vector to const vector"; that sounds like a big non-local change whose correctness would be very difficult to verify. I think (FWIW) that such a fixit would be a very hard sell, but at the same time I admit that I had not even thought of it when I was working on the diagnostic originally.

If you're working in this area, I would also caution you that C++2a is going to completely rewrite the rules in this area. In C++2a, everything that Clang currently warns about is going to be fixed in the standard, so that the Clang warning becomes merely part of -Wc++17-compat. Nobody is currently working on that Clang patch AFAIK. If you're interested in submitting Clang patches in this area, that might be a very high-value (if kind of fuzzy/user-experience/political) patch to work on!

Meanwhile, as of November 2019, the C++2a committee draft still contains unfixed corner cases that Clang doesn't warn about. For example:
https://godbolt.org/z/fBKM-4 (no warning from Clang; C++2a fixes this behavior)
https://godbolt.org/z/EfBCdQ (no warning from Clang; C++2a does NOT fix this behavior)

[courbet]

In D70390#1751751, @Quuxplusone wrote:

that sounds like a big non-local change whose correctness would be very difficult to verify.

Agreed. That's why I did not make this check suggest a fixit, because it's too easy to click "apply" without thinking.

I think (FWIW) that such a fixit would be a very hard sell

I've seen several places in our codebase where it did make quite a big difference performance wise, so they were quite easy to sell :)

Nobody is currently working on that Clang patch AFAIK. If you're interested in submitting Clang patches in this area, that might be a very high-value (if kind of fuzzy/user-experience/political) patch to work on!
Meanwhile, as of November 2019, the C++2a committee draft still contains unfixed corner cases that Clang doesn't warn about. For example:
https://godbolt.org/z/fBKM-4 (no warning from Clang; C++2a fixes this behavior)
https://godbolt.org/z/EfBCdQ (no warning from Clang; C++2a does NOT fix this behavior)

Super interesting, thanks ! I'll see if I have time to work on that.

[courbet]

I'm going to remove the && warning from the check and keep only the const one, as I think these is a consensus on that.

[courbet]

Remove && warnings.

[JonasToth]

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

29

the matchers need only to be registered for c++ and and c++11 and later. See getLangOpts() in other checks.

60

i think isConstQualified could move into the matcher with Matcher<QualType> isConstQualified.

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

12

s/declared/declare

[courbet]

Address comments.

[JonasToth]

LGTM after the variable names are adjusted

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

33

Last nits: all variables do not follow the LLVM convention of camel-casing. Just realized now, sorry

[courbet]

Use LLVM style

[courbet]

Thanks for the review.

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

33

Oh yes right, thanks for the catch.

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyPerformanceModule
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitConversionInLoopCheck.cpp
   InefficientAlgorithmCheck.cpp
   InefficientStringConcatenationCheck.cpp
   InefficientVectorOperationCheck.cpp
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp
+  NoAutomaticMoveCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   PerformanceTidyModule.cpp
   TriviallyDestructibleCheck.cpp
   TypePromotionInMathFnCheck.cpp
   UnnecessaryCopyInitialization.cpp
   UnnecessaryValueParamCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

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

+//===--- 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/NoAutomaticMoveCheck.cpp

+//===--- 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/PerformanceTidyModule.cpp

 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
 #include "ImplicitConversionInLoopCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "InefficientStringConcatenationCheck.h"
 #include "InefficientVectorOperationCheck.h"
 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "NoAutomaticMoveCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "TriviallyDestructibleCheck.h"
 #include "TypePromotionInMathFnCheck.h"
 #include "UnnecessaryCopyInitialization.h"
 #include "UnnecessaryValueParamCheck.h"
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InefficientStringConcatenationCheck>(
         "performance-inefficient-string-concatenation");
     CheckFactories.registerCheck<InefficientVectorOperationCheck>(
         "performance-inefficient-vector-operation");
     CheckFactories.registerCheck<MoveConstArgCheck>(
         "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>(
         "performance-trivially-destructible");
     CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
         "performance-type-promotion-in-math-fn");
     CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
         "performance-unnecessary-copy-initialization");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`llvm-prefer-register-over-unsigned
   them to use ``Register``
 
 - New :doc:`objc-missing-hash
   <clang-tidy/checks/objc-missing-hash>` check.
 
   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.
 
   Finds types that could be made trivially-destructible by removing out-of-line
   defaulted destructor declarations.
 
 - Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.

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

.. toctree::
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-algorithm
    performance-inefficient-string-concatenation
    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
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    portability-simd-intrinsics
    readability-avoid-const-params-in-decls
    readability-braces-around-statements

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

+.. 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

+// 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;
+}