Index: clang-tidy/misc/MoveConstructorInitCheck.h =================================================================== --- clang-tidy/misc/MoveConstructorInitCheck.h +++ 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 namespace clang { namespace tidy { @@ -18,12 +21,24 @@ /// 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 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("move-init") != nullptr) + handleMoveConstructor(Result); + if (Result.Nodes.getNodeAs("movable-param") != nullptr) + handleParamNotMoved(Result); +} + +void MoveConstructorInitCheck::handleParamNotMoved( + const MatchFinder::MatchResult &Result) { + const auto *MovableParam = + Result.Nodes.getNodeAs("movable-param"); + const auto *ConstructorDecl = + Result.Nodes.getNodeAs("ctor-decl"); + const auto *InitArg = Result.Nodes.getNodeAs("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("ctor"); - const auto *Initializer = Result.Nodes.getNodeAs("init"); + const auto *Initializer = Result.Nodes.getNodeAs("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 - 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/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/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/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::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 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/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: 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: 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 struct remove_reference {typedef T type;}; -template struct remove_reference {typedef T type;}; -template struct remove_reference {typedef T type;}; - -template -typename remove_reference::type&& move(T&& arg) { - return static_cast::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 + +// CHECK-FIXES: #include + +template struct remove_reference {typedef T type;}; +template struct remove_reference {typedef T type;}; +template struct remove_reference {typedef T type;}; + +template +typename remove_reference::type&& move(T&& arg) { + return static_cast::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 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_; +};