aaron.ballman updated this revision to Diff 32697.
aaron.ballman added a comment.
In http://reviews.llvm.org/D11784#228764, @alexfh wrote:
> In http://reviews.llvm.org/D11784#227939, @aaron.ballman wrote:
>
> > 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.
>
>
> Determining whether a type is trivially-copyable can be done on the AST level
> without Sema (see QualType::isTriviallyCopyableType).
That should get us closer, but doesn't quite cut it. I think I may be able to
combine this with CXXRecordDecl::isTriviallyCopyable() to get us close enough
to cut down on the number of false positives. Thank you for pointing this out,
it is implemented in this patch.
> > 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.
>
>
> Can you list the list of unique locations?
With the triviality implementation, I now get zero false positives (and zero
true positives) in the Clang and LLVM source base.
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,78 @@
+// 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, is trivially copyable
+struct L : K {
+ L(L &&RHS) : K(RHS) {} // ok
+};
+
+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,77 @@
+//===--- 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");
+
+ // Do not diagnose if the expression used to perform the initialization is a
+ // trivially-copyable type.
+ QualType QT = Initializer->getInit()->getType();
+ if (QT.isTriviallyCopyableType(*Result.Context))
+ return;
+
+ const auto *RD = QT->getAsCXXRecordDecl();
+ if (RD && RD->isTriviallyCopyable())
+ return;
+
+ // 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
