flx updated this revision to Diff 36049.
flx marked 3 inline comments as done.
flx added a comment.
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.
http://reviews.llvm.org/D12839
Files:
clang-tidy/misc/MoveConstructorInitCheck.cpp
clang-tidy/misc/MoveConstructorInitCheck.h
clang-tidy/modernize/PassByValueCheck.cpp
clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
clang-tidy/utils/CMakeLists.txt
clang-tidy/utils/IncludeSorter.cpp
clang-tidy/utils/IncludeSorter.h
clang-tidy/utils/Matchers.h
clang-tidy/utils/TypeTraits.cpp
clang-tidy/utils/TypeTraits.h
docs/clang-tidy/checks/misc-move-constructor-init.rst
test/clang-tidy/misc-move-constructor-init.cpp
Index: test/clang-tidy/misc-move-constructor-init.cpp
===================================================================
--- test/clang-tidy/misc-move-constructor-init.cpp
+++ 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_;
+};
Index: docs/clang-tidy/checks/misc-move-constructor-init.rst
===================================================================
--- docs/clang-tidy/checks/misc-move-constructor-init.rst
+++ docs/clang-tidy/checks/misc-move-constructor-init.rst
@@ -5,3 +5,6 @@
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.
Index: clang-tidy/utils/TypeTraits.h
===================================================================
--- clang-tidy/utils/TypeTraits.h
+++ 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
Index: clang-tidy/utils/TypeTraits.cpp
===================================================================
--- clang-tidy/utils/TypeTraits.cpp
+++ 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
Index: clang-tidy/utils/Matchers.h
===================================================================
--- clang-tidy/utils/Matchers.h
+++ 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
Index: clang-tidy/utils/IncludeSorter.h
===================================================================
--- clang-tidy/utils/IncludeSorter.h
+++ clang-tidy/utils/IncludeSorter.h
@@ -25,6 +25,12 @@
// 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
Index: clang-tidy/utils/IncludeSorter.cpp
===================================================================
--- clang-tidy/utils/IncludeSorter.cpp
+++ clang-tidy/utils/IncludeSorter.cpp
@@ -285,5 +285,14 @@
return Fix;
}
+IncludeSorter::IncludeStyle
+IncludeSorter::toIncludeStyle(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
Index: clang-tidy/utils/CMakeLists.txt
===================================================================
--- clang-tidy/utils/CMakeLists.txt
+++ clang-tidy/utils/CMakeLists.txt
@@ -4,6 +4,7 @@
HeaderGuard.cpp
IncludeInserter.cpp
IncludeSorter.cpp
+ TypeTraits.cpp
LINK_LIBS
clangAST
Index: clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
===================================================================
--- clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
+++ clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
@@ -188,13 +188,11 @@
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) {
Index: clang-tidy/modernize/PassByValueCheck.cpp
===================================================================
--- clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tidy/modernize/PassByValueCheck.cpp
@@ -118,12 +118,11 @@
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) {
Index: clang-tidy/misc/MoveConstructorInitCheck.h
===================================================================
--- clang-tidy/misc/MoveConstructorInitCheck.h
+++ clang-tidy/misc/MoveConstructorInitCheck.h
@@ -11,19 +11,34 @@
#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
Index: clang-tidy/misc/MoveConstructorInitCheck.cpp
===================================================================
--- clang-tidy/misc/MoveConstructorInitCheck.cpp
+++ 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.
@@ -28,14 +56,66 @@
hasAnyConstructorInitializer(
ctorInitializer(withInitializer(constructExpr(hasDeclaration(
constructorDecl(isCopyConstructor()).bind("ctor")
- )))).bind("init")
+ )))).bind("move-init")
)
)), this);
+
+ auto NonConstValueMovableAndExpensiveToCopy =
+ qualType(allOf(unless(pointerType()), unless(isConstQualified()),
+ hasDeclaration(recordDecl(hasMethod(constructorDecl(
+ isMoveConstructor(), unless(isDeleted()))))),
+ matchers::isExpensiveToCopy()));
+ Finder->addMatcher(
+ constructorDecl(
+ allOf(
+ unless(isMoveConstructor()),
+ hasAnyConstructorInitializer(withInitializer(constructExpr(
+ hasDeclaration(constructorDecl(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.
@@ -77,6 +157,15 @@
}
}
+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
-
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
