[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: [PATCH] D11784: [PATCH] clang-tidy check for incorrect move constructor initializers
From: Aaron Ballman <aaron.ballman () gmail ! com>
Date: 2015-08-05 21:57:56
Message-ID: differential-rev-PHID-DREV-nbiqrog5pkqgl7hksvar-req () reviews ! llvm ! org
[Download RAW message or body]
aaron.ballman created this revision.
aaron.ballman added reviewers: klimek, alexfh, djasper.
aaron.ballman added a subscriber: cfe-commits.
This patch adds a new clang-tidy to identify situations where a user is acc=
identally calling a copy constructor instead of a move constructor as part =
of a constructor initializer list. The code is likely to silently "work", b=
ut with surprising results. Consider:
struct B {
B() {}
B(const B&) {}
B(B &&) {}
};
struct D : B {
D() : B() {}
D(const D &RHS) : B(RHS) {} // Correct
D(D &&RHS) : B(RHS) {} // Oops, calls the copy constructor
};
One thing I am not certain of in this patch is how to test it. I have some =
rudimentary tests, but am unable to test the "note:" diagnostics from FileC=
heck (attempting to add any cause the "warning:" diagnostics to not be foun=
d). I suspect this is why clang-tidy.sh exists, but unfortunately, that mea=
ns the tests will not be run on Windows (which is the platform I am develop=
ing on). Suggestions on how to improve the tests are welcome, but for now, =
I'm not testing the note diagnostics.
Another problem I'm not certain how to solve is how to phrase the note diag=
nostics with the constructors are implicitly-defined instead of written. Cu=
rrently, the notes attach to the record declaration. Would it make sense to=
have extra text in that case which tells the user the constructors are imp=
licitly defined? Or is there a better way to surface the notes?
Thanks!
~Aaron
http://reviews.llvm.org/D11784
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstructorBaseCheck.cpp
clang-tidy/misc/MoveConstructorBaseCheck.h
test/clang-tidy/misc-move-constructor-base.cpp
["D11784.31398.patch" (text/x-patch)]
Index: test/clang-tidy/misc-move-constructor-base.cpp
===================================================================
--- test/clang-tidy/misc-move-constructor-base.cpp
+++ test/clang-tidy/misc-move-constructor-base.cpp
@@ -0,0 +1,70 @@
+// RUN: clang-tidy %s -checks=-*,misc-move-constructor-base -- -std=c++14 \
| FileCheck %s +
+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+1]]:16: warning: move constructor initializes base \
class by calling a copy constructor [misc-move-constructor-base] + D(D \
&&RHS) : B(RHS) {} +};
+
+struct E : B {
+ E() : B() {}
+ E(const E &RHS) : B(RHS) {}
+ // CHECK-NOT: warning:
+ E(E &&RHS) : B(move(RHS)) {} // ok
+};
+
+struct F {
+ C M;
+
+ // CHECK-NOT: warning:
+ F(F &&) : M(C()) {}
+};
+
+struct G {
+ G() = default;
+ G(const G&) = default;
+ G(G&&) = delete;
+};
+
+struct H : G {
+ H() = default;
+ H(const H&) = default;
+ // CHECK-NOT: warning:
+ H(H &&RHS) : G(RHS) {} // ok
+};
+
+struct I {
+ I(const I &) = default; // suppresses move constructor creation
+};
+
+struct J : I {
+ // CHECK-NOT: warning:
+ 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-base] + L(L \
&&RHS) : K(RHS) {} +};
Index: clang-tidy/misc/MoveConstructorBaseCheck.h
===================================================================
--- clang-tidy/misc/MoveConstructorBaseCheck.h
+++ clang-tidy/misc/MoveConstructorBaseCheck.h
@@ -0,0 +1,32 @@
+//===--- MoveConstructorBaseCheck.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_MOVECONSTRUCTORBASECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTRUCTORBASECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief The check flags user-defined move constructors that have a
+/// ctor-initializer initializing a base class through a copy constructor
+/// instead of a move constructor.
+class MoveConstructorBaseCheck : public ClangTidyCheck {
+public:
+ MoveConstructorBaseCheck(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_MOVECONSTRUCTORBASECHECK_H
Index: clang-tidy/misc/MoveConstructorBaseCheck.cpp
===================================================================
--- clang-tidy/misc/MoveConstructorBaseCheck.cpp
+++ clang-tidy/misc/MoveConstructorBaseCheck.cpp
@@ -0,0 +1,69 @@
+//===--- MoveConstructorBaseCheck.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 "MoveConstructorBaseCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void MoveConstructorBaseCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ constructorDecl(allOf(
+ isMoveConstructor(),
+ hasAnyConstructorInitializer(allOf(
+ isBaseInitializer(),
+ ctorInitializer(withInitializer(constructExpr(hasDeclaration(
+ constructorDecl(isCopyConstructor()).bind("base_ctor")
+ )))).bind("init")
+ ))
+ )), this);
+}
+
+void MoveConstructorBaseCheck::check(const MatchFinder::MatchResult \
&Result) { + const auto *BaseCopyCtor =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("base_ctor");
+ const auto *Initializer = \
Result.Nodes.getNodeAs<CXXCtorInitializer>("init"); +
+ // Diagnose when the base class has a move constructor available, but \
the + // derived class uses the copy constructor as its ctor-initializer \
instead. + const CXXConstructorDecl *Candidate = nullptr;
+ for (const auto *BCtor : BaseCopyCtor->getParent()->ctors()) {
+ if (BCtor->isMoveConstructor() &&
+ BCtor->getAccess() <= AS_protected &&
+ !BCtor->isDeleted()) {
+ // The base class has a move constructor that is at least accessible \
to + // the derived class.
+ //
+ // FIXME: Determine whether the move constructor is a viable \
candidate + // for the derived constructor's parameter, perhaps \
provide a fixit that + // suggests std::move().
+ Candidate = BCtor;
+ break;
+ }
+ }
+
+ if (Candidate) {
+ // There's a base class move constructor candidate that the caller \
probably + // intended to call instead.
+ diag(Initializer->getSourceLocation(),
+ "move constructor initializes base class by calling a copy \
constructor"); + diag(BaseCopyCtor->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 "MoveConstructorBaseCheck.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<MoveConstructorBaseCheck>(
+ "misc-move-constructor-base");
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
+ MoveConstructorBaseCheck.cpp
NoexceptMoveConstructorCheck.cpp
StaticAssertCheck.cpp
SwappedArgumentsCheck.cpp
[Attachment #4 (text/plain)]
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic