Improved the misc-move-constructor-init check to identify arguments that are passed by value but copy assigned to class data members when the non-deleted move constructor is a better fit.

Patch by Felix Berger!

llvm-svn: 249429

clang-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.cpp

@@ -8,14 +8,42 @@
 //===----------------------------------------------------------------------===//
 
 #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.
@@ -31,13 +59,65 @@ void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
                         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.
@@ -79,6 +159,15 @@ void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
   }
 }
 
+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-tools-extra/clang-tidy/misc/MoveConstructorInitCheck.h

@@ -11,6 +11,9 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H
 
 #include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+#include <memory>
 
 namespace clang {
 namespace tidy {
@@ -18,12 +21,24 @@ 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

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

@@ -118,12 +118,11 @@ collectParamDecls(const CXXConstructorDecl *Ctor,
 
 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) {

clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp

@@ -189,13 +189,11 @@ static bool checkTokenIsAutoPtr(SourceLocation TokenStart,
 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) {

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

@@ -4,6 +4,7 @@ add_clang_library(clangTidyUtils
   HeaderGuard.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
+  TypeTraits.cpp
 
   LINK_LIBS
   clangAST

clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp

@@ -285,5 +285,14 @@ FixItHint IncludeSorter::CreateFixIt(SourceRange EditRange,
   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-tools-extra/clang-tidy/utils/IncludeSorter.h

@@ -25,6 +25,12 @@ 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

clang-tools-extra/clang-tidy/utils/Matchers.h

@@ -0,0 +1,28 @@
+//===--- 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-tools-extra/clang-tidy/utils/TypeTraits.cpp

@@ -0,0 +1,34 @@
+//===--- 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

clang-tools-extra/clang-tidy/utils/TypeTraits.h

@@ -0,0 +1,27 @@
+//===--- 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-tools-extra/docs/clang-tidy/checks/misc-move-constructor-init.rst

@@ -5,3 +5,6 @@ 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.

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

@@ -1,78 +1,145 @@
-// 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_;
+};