aaron.ballman updated this revision to Diff 32559.
aaron.ballman added a comment.
Addressed review comments. I re-ran the updated patch against LLVM and Clang,
and there were some more false positives that I would like to address if
possible. It seems my previous run against the source base was before expanding
the scope of the patch to include more than just base class initialization
(sorry for the earlier misinformation).
1. SourceMgr.h:58 is an example where the checker issues a diagnostic (for
IncludeLoc), but given the triviality of the type, I don't see a reason to
diagnose. However, this requires support from Sema, so I think a FIXME may be
the best I can do. Note, adding a call to std::move() in these instances is not
wrong, it's just not particularly useful.
2. we should not be warning on anything an implicit constructor does. For
instance LiveQueryResult is triggering this because of SlotIndex. This should
be fixed with this patch.
Running over Clang and LLVM, there are 7 distinct false positives (repeated due
to being in header files) and they all relate to triviality. The total false
positive count was 832 of which two warnings (SourceMgr.h and Preprocessor.h)
accounted for probably close to 90% of the diagnostics. This time around there
were no true positives.
http://reviews.llvm.org/D11784
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstructorInitCheck.cpp
clang-tidy/misc/MoveConstructorInitCheck.h
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
@@ -0,0 +1,79 @@
+// RUN: clang-tidy %s -checks=-*,misc-move-constructor-init -- -std=c++14 | FileCheck %s -implicit-check-not="{{warning|error}}:"
+
+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: :[[@LINE+3]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-init]
+ // CHECK: 19:3: note: copy constructor being called
+ // CHECK: 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
+struct L : K {
+ // CHECK: :[[@LINE+1]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-init]
+ L(L &&RHS) : K(RHS) {}
+};
+
+struct M {
+ B Mem;
+ // CHECK: :[[@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)) {}
+};
Index: clang-tidy/misc/MoveConstructorInitCheck.h
===================================================================
--- clang-tidy/misc/MoveConstructorInitCheck.h
+++ clang-tidy/misc/MoveConstructorInitCheck.h
@@ -0,0 +1,32 @@
+//===--- 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"
+
+namespace clang {
+namespace tidy {
+
+/// \brief 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.
+class MoveConstructorInitCheck : public ClangTidyCheck {
+public:
+ MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORINITCHECK_H
Index: clang-tidy/misc/MoveConstructorInitCheck.cpp
===================================================================
--- clang-tidy/misc/MoveConstructorInitCheck.cpp
+++ clang-tidy/misc/MoveConstructorInitCheck.cpp
@@ -0,0 +1,67 @@
+//===--- 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 "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ constructorDecl(unless(isImplicit()), allOf(
+ isMoveConstructor(),
+ hasAnyConstructorInitializer(
+ ctorInitializer(withInitializer(constructExpr(hasDeclaration(
+ constructorDecl(isCopyConstructor()).bind("ctor")
+ )))).bind("init")
+ )
+ )), this);
+}
+
+void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *CopyCtor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
+ const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("init");
+
+ // Diagnose when the class type has a move constructor available, but the
+ // ctor-initializer uses the copy constructor instead.
+ const CXXConstructorDecl *Candidate = nullptr;
+ for (const auto *Ctor : CopyCtor->getParent()->ctors()) {
+ if (Ctor->isMoveConstructor() && Ctor->getAccess() <= AS_protected &&
+ !Ctor->isDeleted()) {
+ // The type has a move constructor that is at least accessible to the
+ // initializer.
+ //
+ // FIXME: Determine whether the move constructor is a viable candidate
+ // for the ctor-initializer, perhaps provide a fixit that suggests
+ // using std::move().
+ Candidate = Ctor;
+ break;
+ }
+ }
+
+ if (Candidate) {
+ // There's a move constructor candidate that the caller probably intended
+ // to call instead.
+ diag(Initializer->getSourceLocation(),
+ "move constructor initializes %0 by calling a copy constructor")
+ << (Initializer->isBaseInitializer() ? "base class" : "class member");
+ diag(CopyCtor->getLocation(), "copy constructor being called",
+ DiagnosticIDs::Note);
+ diag(Candidate->getLocation(), "candidate move constructor here",
+ DiagnosticIDs::Note);
+ }
+}
+
+} // namespace tidy
+} // namespace clang
+
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -18,6 +18,7 @@
#include "InefficientAlgorithmCheck.h"
#include "MacroParenthesesCheck.h"
#include "MacroRepeatedSideEffectsCheck.h"
+#include "MoveConstructorInitCheck.h"
#include "NoexceptMoveConstructorCheck.h"
#include "StaticAssertCheck.h"
#include "SwappedArgumentsCheck.h"
@@ -50,6 +51,8 @@
"misc-macro-parentheses");
CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
"misc-macro-repeated-side-effects");
+ CheckFactories.registerCheck<MoveConstructorInitCheck>(
+ "misc-move-constructor-init");
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
"misc-noexcept-move-constructor");
CheckFactories.registerCheck<StaticAssertCheck>(
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -10,6 +10,7 @@
MacroParenthesesCheck.cpp
MacroRepeatedSideEffectsCheck.cpp
MiscTidyModule.cpp
+ MoveConstructorInitCheck.cpp
NoexceptMoveConstructorCheck.cpp
StaticAssertCheck.cpp
SwappedArgumentsCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
