Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[flx]

Summary

Add matcher that identifies arguments that are passed by value, are copy assigned to a class field and have a non-deleted move constructor. If the type is also expensive to copy issue diagnostic message that the field can be move constructed.

[aaron.ballman]

There is a -Wpessimizing-move frontend warning that Richard added not too long ago, which tells the user to remove calls to std::move() because they pessimize the code. The new functionality you are looking to add is basically the opposite: it tells the user to add std::move() because the code is currently pessimized due to copies. Given how general that concept is (it doesn't just appertain to constructor initializations), I wonder if this makes more sense as a frontend warning like -Wpessimizing-copy.

Richard (both of you), what do you think?

~Aaron

clang-tidy/misc/MoveConstructorInitCheck.cpp

36

Elide braces, here and elsewhere.

40

Can turn this into a return statement directly instead of an if.

42

Why do you need classHasTrivialCopyAndDestroy() when you're already checking if it's a trivially copyable type?

48

Should return unsigned, please.

54

You don't actually need Matches, you can call match().size() instead.

56

Should be *Initializer instead of * Initializer.

108

Perhaps: "argument can be moved to avoid a copy" instead?

test/clang-tidy/misc-move-constructor-init.cpp

7

Formatting

8

Why not = default?

36

Should also have a test for scalars

[alexfh]

Oops, forgot to hit "Submit" yesterday. Sorry for the delay.

clang-tidy/misc/MoveConstructorInitCheck.cpp

32

These helper functions can (and will) be useful for other checks as well. Let's move them to clang-tidy/utils/ . We can add TypeTraits.h for functions like classHasTrivialCopyAndDestroy, isExpensiveTypeToCopy, etc. And a separate header (Matchers.h, for example) for the matchers that are not suitable for the ASTMatchers.h.

88

nit: It's common to omit braces around single-line statements in LLVM/Clang code.

108

nit: Please remove the trailing period.

108

Does anything stop us from suggesting fixes here (inserting "std::move()" around the argument and #include <utility>, if it's no there yet)?

clang-tidy/misc/MoveConstructorInitCheck.h

24

I suggest updating the corresponding .rst file instead and adding the URL of the documentation in the comment along with a short summary of what the check does.

For example:

/// Flag missing move opportunities in constructor initializer lists.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/misc-move-constructor-init.html

[flx]

clang-tidy/misc/MoveConstructorInitCheck.cpp

42

We also want to catch types that have non-trivial destructors which would be executed when the temporary copy goes out of scope.

48

Done. Is this an llvm convention?

108

How would I tread in the IncludeOrder style (i.e. Google vs LLVM)? Should this be a flag shared by all of ClangTidy or specific to this check?

test/clang-tidy/misc-move-constructor-init.cpp

8

We need to make the type non-trivially copyable by our definition above.

36

Added.

[aaron.ballman]

clang-tidy/misc/MoveConstructorInitCheck.cpp

11

Errr, I'm not certain that we typically use relative paths for this sort of thing. I see we do use relative paths for other things in clang-tidy, but this is definitely an uncommon thing in the rest of the code base (outside of test or example code).

Not saying you should change anything here, but we may want to consider changing the paths at some point so we are more in line with the rest of the LLVM projects in not using relative include paths.

clang-tidy/utils/Matchers.h

2

You are missing header include guards.

clang-tidy/utils/TypeTraits.cpp

30

This isn't needed because Type.isTriviallyCopyableType() already returns false for this case.

33

No need to check for isScalarType() because isTriviallyCopyableType() already does that correctly.

clang-tidy/utils/TypeTraits.h

23

I think this is an implementation detail more than a trait we want to expose.

test/clang-tidy/misc-move-constructor-init.cpp

37

Sorry to have not caught this earlier, but can you also add a test case for dependent types to make sure they remain negative?

[alexfh]

clang-tidy/misc/MoveConstructorInitCheck.cpp

108

Other checks have separate options for the include style (see PassByValueCheck.cpp, for example), so this one should have its own option too. If we ever decide to introduce a common option instead, it will be easier to do this, if all usages of the IncludeInserter do the same thing with the include style.

One thing I would change though, is to add the IncludeStyle <-> string conversion functions instead of doing this in each check.

[flx]

I changed the check to also produce a fix that wraps the argument in std::move().

When I modified the test include -isystem %S/Inputs/Headers it broke and only produces warnings but no fixes anymore. Is there a subtle difference in how tests with includes need to be invoked?

[alexfh]

In D12839#254384, @flx wrote:

I changed the check to also produce a fix that wraps the argument in std::move().

When I modified the test include -isystem %S/Inputs/Headers it broke and only produces warnings but no fixes anymore. Is there a subtle difference in how tests with includes need to be invoked?

Did you add the files you're including (j.h, s.h)? If not, this will result in compilation errors and clang-tidy by default won't apply fixes, if the code doesn't compile. In any case, they should be present in the patch as well.

clang-tidy/misc/MoveConstructorInitCheck.cpp

112

/*IsAngled=*/true)) {

clang-tidy/modernize/PassByValueCheck.cpp

121–122

clang-format?

clang-tidy/modernize/ReplaceAutoPtrCheck.cpp

192–193

clang-format?

clang-tidy/utils/IncludeSorter.h

29

nit: The current name gives no hints at what is being converted to IncludeStyle, but parseIncludeStyle would make it clear that the source is a string.

[flx]

I had to convert the line endings of mis-move-constructor-init.cpp to unix style otherwise the test would not correctly work. This took a long time to debug since the checks failed complaining that error messages that look exactly the same didn't match.

Let me know if this needs to be converted back to DOS line endings, but it looks like most other tests have unix line endings.

[alexfh]

Looks good. Please fix the patch and I can commit it for you.

clang-tidy/utils/IncludeSorter.cpp

289

Did you forget to update the name here?

[alexfh]

In D12839#256179, @flx wrote:

I had to convert the line endings of mis-move-constructor-init.cpp to unix style otherwise the test would not correctly work. This took a long time to debug since the checks failed complaining that error messages that look exactly the same didn't match.

Let me know if this needs to be converted back to DOS line endings, but it looks like most other tests have unix line endings.

All source and test files should have svn:eol-style=native property set to avoid this kind of an issue. I have no idea why is it not done on all llvm/clang sources.

[flx]

Thanks for catching the last issues, Alex. I rebuilt and ran all tests using llvm-lit -v . for clang-tidy and they pass.

[alexfh]

The patch doesn't apply cleanly. Please update it.

[flx]

Updated patch to new cxx* matcher variants.

[aaron.ballman]

LGTM!

Do you have commit access, or would you like me to commit on your behalf?

[flx]

I don't have access, so that'd be great if you or Alex could submit the patch, thanks!

clang-tidy/misc/MoveConstructorInitCheck.cpp

11

Leaving this as is for now as all other ClangTidy specific includes I could find were relative as well. Not sure if non-relative includes require extra work in the build rules.

108

Done. I moved the conversion into IncludeSorter where the the enum is defined.

clang-tidy/modernize/PassByValueCheck.cpp

121–122

This is how clang-format formatted it. I used the following invocation:

clang-format --style=LLVM MoveConstructorInitCheck.cpp > formatted

And then diffed to only update format changes in code I added.

clang-tidy/utils/IncludeSorter.h

29

Done, although the argument type uniquely identifies a function.

clang-tidy/utils/Matchers.h

2

Thanks for catching that. Done.

clang-tidy/utils/TypeTraits.cpp

33

Done. Also demorganized the expression to make it easier to understand.

clang-tidy/utils/TypeTraits.h

23

Removed from header.

[aaron.ballman]

Thank you for the patch! I've commit in r249429.

[malcolm.parsons]

The modernize-pass-by-value check does the same thing:

test/clang-tidy/misc-move-constructor-init.cpp:98:12: warning: pass by value and use std::move [modernize-pass-by-value]
  Positive(Movable M) : M_(M) {}
           ^
                           std::move( )
test/clang-tidy/misc-move-constructor-init.cpp:98:28: warning: value argument 'M' can be moved to avoid copy [misc-move-constructor-init]
  Positive(Movable M) : M_(M) {}
                           ^

Do we need two checks for this?

[aaron.ballman]

In D12839#582828, @malcolm.parsons wrote:

The modernize-pass-by-value check does the same thing:

test/clang-tidy/misc-move-constructor-init.cpp:98:12: warning: pass by value and use std::move [modernize-pass-by-value]
  Positive(Movable M) : M_(M) {}
           ^
                           std::move( )
test/clang-tidy/misc-move-constructor-init.cpp:98:28: warning: value argument 'M' can be moved to avoid copy [misc-move-constructor-init]
  Positive(Movable M) : M_(M) {}
                           ^

Do we need two checks for this?

This overlap is unfortunate. misc-move-constructor-init is for move constructor initializer lists which accidentally initialize a member or base through a copy constructor rather than a move constructor. Consider:

struct B { B(B&&); B(const B&); };
struct D {
  D(D &&d) : B(d) {} // Oops, calling B(const B&) not B(B&&)
  D(const D &d) : B(d) {}

modernize-pass-by-value is slightly different in that it is concerned with the parameter of an arbitrary function being something that is movable if you pass it by value instead of passing it by reference.

So misc-move-constructor-init cares about move constructor initializer lists while modernize-pass-by-value cares about the parameters of a function.

[malcolm.parsons]

In D12839#585256, @aaron.ballman wrote:

This overlap is unfortunate. misc-move-constructor-init is for move constructor initializer lists which accidentally initialize a member or base through a copy constructor rather than a move constructor. Consider:

The main check is, but the additional sub-check added in this patch is already handled by modernize-pass-by-value.

modernize-pass-by-value is slightly different in that it is concerned with the parameter of an arbitrary function being something that is movable if you pass it by value instead of passing it by reference.

The doc for modernize-pass-by-value says:

Currently, only constructors are transformed to make use of pass-by-value.
Contributions that handle other situations are welcome!

clang-tidy/misc/MoveConstructorInitCheck.h

 //===--- MoveConstructorInitCheck.h - clang-tidy-----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H
 
 #include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+#include <memory>
 
 namespace clang {
 namespace tidy {
 
 /// The check flags user-defined move constructors that have a ctor-initializer
 /// initializing a member or base class through a copy constructor instead of a
 /// move constructor.
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-move-constructor-init.html
 class MoveConstructorInitCheck : public ClangTidyCheck {
 public:
-  MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(clang::CompilerInstance &Compiler) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  void
+  handleMoveConstructor(const ast_matchers::MatchFinder::MatchResult &Result);
+  void
+  handleParamNotMoved(const ast_matchers::MatchFinder::MatchResult &Result);
+
+  std::unique_ptr<IncludeInserter> Inserter;
+  const IncludeSorter::IncludeStyle IncludeStyle;
 };
 
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H

clang-tidy/misc/MoveConstructorInitCheck.cpp

 //===--- MoveConstructorInitCheck.cpp - clang-tidy-------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MoveConstructorInitCheck.h"
+#include "../utils/Matchers.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
+namespace {
+
+unsigned int
+parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
+                             const CXXConstructorDecl &ConstructorDecl,
+                             ASTContext &Context) {
+  unsigned int Occurrences = 0;
+  auto AllDeclRefs =
+      findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam)))));
+  Occurrences += match(AllDeclRefs, *ConstructorDecl.getBody(), Context).size();
+  for (const auto *Initializer : ConstructorDecl.inits()) {
+    Occurrences += match(AllDeclRefs, *Initializer->getInit(), Context).size();
+  }
+  return Occurrences;
+}
+
+} // namespace
+
+MoveConstructorInitCheck::MoveConstructorInitCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeStyle(IncludeSorter::parseIncludeStyle(
+          Options.get("IncludeStyle", "llvm"))) {}
+
 void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++11; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus11)
     return;
 
   Finder->addMatcher(
       cxxConstructorDecl(
           unless(isImplicit()),
           allOf(isMoveConstructor(),
                 hasAnyConstructorInitializer(
                     cxxCtorInitializer(
                         withInitializer(cxxConstructExpr(hasDeclaration(
                             cxxConstructorDecl(isCopyConstructor())
                                 .bind("ctor")))))
-                        .bind("init")))),
+                        .bind("move-init")))),
+      this);
+
+  auto NonConstValueMovableAndExpensiveToCopy =
+      qualType(allOf(unless(pointerType()), unless(isConstQualified()),
+                     hasDeclaration(cxxRecordDecl(hasMethod(cxxConstructorDecl(
+                         isMoveConstructor(), unless(isDeleted()))))),
+                     matchers::isExpensiveToCopy()));
+  Finder->addMatcher(
+      cxxConstructorDecl(
+          allOf(
+              unless(isMoveConstructor()),
+              hasAnyConstructorInitializer(withInitializer(cxxConstructExpr(
+                  hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
+                  hasArgument(
+                      0, declRefExpr(
+                             to(parmVarDecl(
+                                    hasType(
+                                        NonConstValueMovableAndExpensiveToCopy))
+                                    .bind("movable-param")))
+                             .bind("init-arg")))))))
+          .bind("ctor-decl"),
       this);
 }
 
 void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
+  if (Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init") != nullptr)
+    handleMoveConstructor(Result);
+  if (Result.Nodes.getNodeAs<ParmVarDecl>("movable-param") != nullptr)
+    handleParamNotMoved(Result);
+}
+
+void MoveConstructorInitCheck::handleParamNotMoved(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MovableParam =
+      Result.Nodes.getNodeAs<ParmVarDecl>("movable-param");
+  const auto *ConstructorDecl =
+      Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor-decl");
+  const auto *InitArg = Result.Nodes.getNodeAs<DeclRefExpr>("init-arg");
+  // If the parameter is referenced more than once it is not safe to move it.
+  if (parmVarDeclRefExprOccurences(*MovableParam, *ConstructorDecl,
+                                   *Result.Context) > 1)
+    return;
+  auto DiagOut =
+      diag(InitArg->getLocStart(), "value argument can be moved to avoid copy");
+  DiagOut << FixItHint::CreateReplacement(
+      InitArg->getSourceRange(),
+      (Twine("std::move(") + MovableParam->getName() + ")").str());
+  if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+          Result.SourceManager->getFileID(InitArg->getLocStart()), "utility",
+          /*IsAngled=*/true)) {
+    DiagOut << *IncludeFixit;
+  }
+}
+
+void MoveConstructorInitCheck::handleMoveConstructor(
+    const MatchFinder::MatchResult &Result) {
   const auto *CopyCtor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
-  const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("init");
+  const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init");
 
   // Do not diagnose if the expression used to perform the initialization is a
   // trivially-copyable type.
   QualType QT = Initializer->getInit()->getType();
   if (QT.isTriviallyCopyableType(*Result.Context))
     return;
   
   const auto *RD = QT->getAsCXXRecordDecl();
diag(Initializer->getSourceLocation(),
         << (Initializer->isBaseInitializer() ? "base class" : "class member");
     diag(CopyCtor->getLocation(), "copy constructor being called",
          DiagnosticIDs::Note);
     diag(Candidate->getLocation(), "candidate move constructor here",
          DiagnosticIDs::Note);
   }
 }
 
+void MoveConstructorInitCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Inserter.reset(new IncludeInserter(Compiler.getSourceManager(),
+                                     Compiler.getLangOpts(), IncludeStyle));
+  Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void MoveConstructorInitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle", IncludeSorter::toString(IncludeStyle));
+}
+
 } // namespace tidy
 } // namespace clang
-

clang-tidy/modernize/PassByValueCheck.cpp

collectParamDecls(const CXXConstructorDecl *Ctor,
 
   for (const FunctionDecl *Redecl : Ctor->redecls())
     Results.push_back(Redecl->getParamDecl(ParamIdx));
   return Results;
 }
 
 PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      IncludeStyle(Options.get("IncludeStyle", "llvm") == "llvm" ?
-                   IncludeSorter::IS_LLVM : IncludeSorter::IS_Google) {}
+      IncludeStyle(IncludeSorter::parseIncludeStyle(
+          Options.get("IncludeStyle", "llvm"))) {}
 
 void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "IncludeStyle",
-                IncludeStyle == IncludeSorter::IS_LLVM ? "llvm" : "google");
+  Options.store(Opts, "IncludeStyle", IncludeSorter::toString(IncludeStyle));
 }
 
 void PassByValueCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (getLangOpts().CPlusPlus) {
     Finder->addMatcher(
         cxxConstructorDecl(

clang-tidy/modernize/ReplaceAutoPtrCheck.cpp

static bool checkTokenIsAutoPtr(SourceLocation TokenStart,
   StringRef Res = Lexer::getSpelling(TokenStart, Buffer, SM, LO, &Invalid);
 
   return (!Invalid && Res == "auto_ptr");
 }
 
 ReplaceAutoPtrCheck::ReplaceAutoPtrCheck(StringRef Name,
                                          ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      IncludeStyle(Options.get("IncludeStyle", "llvm") == "llvm"
-                       ? IncludeSorter::IS_LLVM
-                       : IncludeSorter::IS_Google) {}
+      IncludeStyle(IncludeSorter::parseIncludeStyle(
+          Options.get("IncludeStyle", "llvm"))) {}
 
 void ReplaceAutoPtrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "IncludeStyle",
-                IncludeStyle == IncludeSorter::IS_LLVM ? "llvm" : "google");
+  Options.store(Opts, "IncludeStyle", IncludeSorter::toString(IncludeStyle));
 }
 
 void ReplaceAutoPtrCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (getLangOpts().CPlusPlus) {
     Finder->addMatcher(makeAutoPtrTypeLocMatcher(), this);
     Finder->addMatcher(makeAutoPtrUsingDeclMatcher(), this);

clang-tidy/utils/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyUtils
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
+  TypeTraits.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/utils/IncludeSorter.h

 // names of the inclusions in a given source file being processed and generate
 // the necessary commands to sort the inclusions according to the precedence
 // enocded in IncludeKinds.
 class IncludeSorter {
 public:
   // Supported include styles.
   enum IncludeStyle { IS_LLVM = 0, IS_Google = 1 };
 
+  // Converts "llvm" to IS_LLVM, otherwise returns IS_Google.
+  static IncludeStyle parseIncludeStyle(const std::string &Value);
+
+  // Converts IncludeStyle to string representation.
+  static StringRef toString(IncludeStyle Style);
+
   // The classifications of inclusions, in the order they should be sorted.
   enum IncludeKinds {
     IK_MainTUInclude = 0,    // e.g. #include "foo.h" when editing foo.cc
     IK_CSystemInclude = 1,   // e.g. #include <stdio.h>
     IK_CXXSystemInclude = 2, // e.g. #include <vector>
     IK_NonSystemInclude = 3, // e.g. #include "bar.h"
     IK_InvalidInclude = 4    // total number of valid IncludeKinds
   };

clang-tidy/utils/IncludeSorter.cpp

 FixItHint IncludeSorter::CreateFixIt(SourceRange EditRange,
                                      const std::string &NewText) {
   FixItHint Fix;
   Fix.RemoveRange = CharSourceRange::getCharRange(EditRange);
   Fix.CodeToInsert = NewText;
   return Fix;
 }
 
+IncludeSorter::IncludeStyle
+IncludeSorter::parseIncludeStyle(const std::string &Value) {
+  return Value == "llvm" ? IS_LLVM : IS_Google;
+}
+
+StringRef IncludeSorter::toString(IncludeStyle Style) {
+  return Style == IS_LLVM ? "llvm" : "google";
+}
+
 } // namespace tidy
 } // namespace clang

clang-tidy/utils/Matchers.h

+//===--- Matchers.h - clang-tidy-------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "TypeTraits.h"
+
+namespace clang {
+namespace tidy {
+namespace matchers {
+
+AST_MATCHER(QualType, isExpensiveToCopy) {
+  return type_traits::isExpensiveToCopy(Node, Finder->getASTContext());
+}
+
+} // namespace matchers
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H

clang-tidy/utils/TypeTraits.h

+//===--- TypeTraits.h - clang-tidy-------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
+
+namespace clang {
+namespace tidy {
+namespace type_traits {
+
+// \brief Returns true If \c Type is expensive to copy.
+bool isExpensiveToCopy(QualType Type, ASTContext &Context);
+
+} // type_traits
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H

clang-tidy/utils/TypeTraits.cpp

+//===--- TypeTraits.cpp - clang-tidy---------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TypeTraits.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+
+namespace clang {
+namespace tidy {
+namespace type_traits {
+
+namespace {
+bool classHasTrivialCopyAndDestroy(QualType Type) {
+  auto *Record = Type->getAsCXXRecordDecl();
+  return Record && Record->hasDefinition() &&
+         !Record->hasNonTrivialCopyConstructor() &&
+         !Record->hasNonTrivialDestructor();
+}
+} // namespace
+
+bool isExpensiveToCopy(QualType Type, ASTContext &Context) {
+  return !Type.isTriviallyCopyableType(Context) &&
+      !classHasTrivialCopyAndDestroy(Type);
+}
+
+} // type_traits
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/misc-move-constructor-init.rst

 misc-move-constructor-init
 ==========================
 
 
 The check flags user-defined move constructors that have a ctor-initializer
 initializing a member or base class through a copy constructor instead of a
 move constructor.
+
+It also flags constructor arguments that are passed by value, have a non-deleted
+move-constructor and are assigned to a class field by copy construction.

test/clang-tidy/misc-move-constructor-init.cpp

-// RUN: %python %S/check_clang_tidy.py %s misc-move-constructor-init %t
-
-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 <typename T>
-typename remove_reference<T>::type&& move(T&& arg) {
-  return static_cast<typename remove_reference<T>::type&&>(arg);
-}
-
-struct C {
-  C() = default;
-  C(const C&) = default;
-};
-
-struct B {
-  B() {}
-  B(const B&) {}
-  B(B &&) {}
-};
-
-struct D : B {
-  D() : B() {}
-  D(const D &RHS) : B(RHS) {}
-  // CHECK-MESSAGES: :[[@LINE+3]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-init]
-  // CHECK-MESSAGES: 19:3: note: copy constructor being called
-  // CHECK-MESSAGES: 20:3: note: candidate move constructor here
-  D(D &&RHS) : B(RHS) {}
-};
-
-struct E : B {
-  E() : B() {}
-  E(const E &RHS) : B(RHS) {}
-  E(E &&RHS) : B(move(RHS)) {} // ok
-};
-
-struct F {
-  C M;
-
-  F(F &&) : M(C()) {} // ok
-};
-
-struct G {
-  G() = default;
-  G(const G&) = default;
-  G(G&&) = delete;
-};
-
-struct H : G {
-  H() = default;
-  H(const H&) = default;
-  H(H &&RHS) : G(RHS) {} // ok
-};
-
-struct I {
-  I(const I &) = default; // suppresses move constructor creation
-};
-
-struct J : I {
-  J(J &&RHS) : I(RHS) {} // ok
-};
-
-struct K {}; // Has implicit copy and move constructors, is trivially copyable
-struct L : K {
-  L(L &&RHS) : K(RHS) {} // ok
-};
-
-struct M {
-  B Mem;
-  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: move constructor initializes class member by calling a copy constructor [misc-move-constructor-init]
-  M(M &&RHS) : Mem(RHS.Mem) {}
-};
-
-struct N {
-  B Mem;
-  N(N &&RHS) : Mem(move(RHS.Mem)) {}
-};
+// RUN: %python %S/check_clang_tidy.py %s misc-move-constructor-init %t -- -std=c++11 -isystem %S/Inputs/Headers
+
+#include <s.h>
+
+// CHECK-FIXES: #include <utility>
+
+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 <typename T>
+typename remove_reference<T>::type&& move(T&& arg) {
+  return static_cast<typename remove_reference<T>::type&&>(arg);
+}
+
+struct C {
+  C() = default;
+  C(const C&) = default;
+};
+
+struct B {
+  B() {}
+  B(const B&) {}
+  B(B &&) {}
+};
+
+struct D : B {
+  D() : B() {}
+  D(const D &RHS) : B(RHS) {}
+  // CHECK-MESSAGES: :[[@LINE+3]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-init]
+  // CHECK-MESSAGES: 23:3: note: copy constructor being called
+  // CHECK-MESSAGES: 24:3: note: candidate move constructor here
+  D(D &&RHS) : B(RHS) {}
+};
+
+struct E : B {
+  E() : B() {}
+  E(const E &RHS) : B(RHS) {}
+  E(E &&RHS) : B(move(RHS)) {} // ok
+};
+
+struct F {
+  C M;
+
+  F(F &&) : M(C()) {} // ok
+};
+
+struct G {
+  G() = default;
+  G(const G&) = default;
+  G(G&&) = delete;
+};
+
+struct H : G {
+  H() = default;
+  H(const H&) = default;
+  H(H &&RHS) : G(RHS) {} // ok
+};
+
+struct I {
+  I(const I &) = default; // suppresses move constructor creation
+};
+
+struct J : I {
+  J(J &&RHS) : I(RHS) {} // ok
+};
+
+struct K {}; // Has implicit copy and move constructors, is trivially copyable
+struct L : K {
+  L(L &&RHS) : K(RHS) {} // ok
+};
+
+struct M {
+  B Mem;
+  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: move constructor initializes class member by calling a copy constructor [misc-move-constructor-init]
+  M(M &&RHS) : Mem(RHS.Mem) {}
+};
+
+struct N {
+  B Mem;
+  N(N &&RHS) : Mem(move(RHS.Mem)) {}
+};
+
+struct Movable {
+  Movable(Movable &&) = default;
+  Movable(const Movable &) = default;
+  Movable &operator=(const Movable &) = default;
+  ~Movable() {}
+};
+
+struct TriviallyCopyable {
+  TriviallyCopyable() = default;
+  TriviallyCopyable(TriviallyCopyable &&) = default;
+  TriviallyCopyable(const TriviallyCopyable &) = default;
+};
+
+struct Positive {
+  Positive(Movable M) : M_(M) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: value argument can be moved to avoid copy [misc-move-constructor-init]
+  // CHECK-FIXES: Positive(Movable M) : M_(std::move(M)) {}
+  Movable M_;
+};
+
+struct NegativeMultipleInitializerReferences {
+  NegativeMultipleInitializerReferences(Movable M) : M_(M), n_(M) {}
+  Movable M_;
+  Movable n_;
+};
+
+struct NegativeReferencedInConstructorBody {
+  NegativeReferencedInConstructorBody(Movable M) : M_(M) { M_ = M; }
+  Movable M_;
+};
+
+struct NegativeParamTriviallyCopyable {
+  NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
+  NegativeParamTriviallyCopyable(int I) : I_(I) {}
+
+  TriviallyCopyable T_;
+  int I_;
+};
+
+template <typename T> struct NegativeDependentType {
+  NegativeDependentType(T Value) : T_(Value) {}
+  T T_;
+};
+
+struct NegativeNotPassedByValue {
+  NegativeNotPassedByValue(const Movable &M) : M_(M) {}
+  NegativeNotPassedByValue(const Movable M) : M_(M) {}
+  NegativeNotPassedByValue(Movable &M) : M_(M) {}
+  NegativeNotPassedByValue(Movable *M) : M_(*M) {}
+  NegativeNotPassedByValue(const Movable *M) : M_(*M) {}
+  Movable M_;
+};
+
+struct Immovable {
+  Immovable(const Immovable &) = default;
+  Immovable(Immovable &&) = delete;
+};
+
+struct NegativeImmovableParameter {
+  NegativeImmovableParameter(Immovable I) : I_(I) {}
+  Immovable I_;
+};