flx updated this revision to Diff 35826.
flx marked 7 inline comments as done.
flx added a comment.
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?
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,4 +1,8 @@
-// RUN: %python %S/check_clang_tidy.py %s misc-move-constructor-init %t
+// RUN: %python %S/check_clang_tidy.py %s misc-move-constructor-init %t -- -isystem %S/Inputs/Headers
+
+#include "j.h"
+#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;};
@@ -76,3 +80,66 @@
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 toIncludeStyle(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,10 @@
ReplaceAutoPtrCheck::ReplaceAutoPtrCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- IncludeStyle(Options.get("IncludeStyle", "llvm") == "llvm"
- ? IncludeSorter::IS_LLVM
- : IncludeSorter::IS_Google) {}
+ IncludeStyle(IncludeSorter::toIncludeStyle(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,10 @@
PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- IncludeStyle(Options.get("IncludeStyle", "llvm") == "llvm" ?
- IncludeSorter::IS_LLVM : IncludeSorter::IS_Google) {}
+ IncludeStyle(IncludeSorter::toIncludeStyle(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::toIncludeStyle(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",
+ 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
