Extend base::Callback to have a move-only variant

base/callback_internal.cc

Index: base/callback_internal.cc
diff --git a/base/callback_internal.cc b/base/callback_internal.cc
index 2553fe7e1b64bf20c67dde90800377dc44b58fc2..1b05bb8a4b98a2565643795b91a80bbb47f9a587 100644
--- a/base/callback_internal.cc
+++ b/base/callback_internal.cc
@@ -18,29 +18,66 @@ void BindStateBase::Release() {
     destructor_(this);
 }
 
-CallbackBase::CallbackBase(const CallbackBase& c) = default;
-CallbackBase& CallbackBase::operator=(const CallbackBase& c) = default;
+BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::CallbackBase(CallbackBase&& c)

[dcheng, 2016/03/02 08:02:36]

Huh. It's surprising to see _EXPORT macros in a .cc file.

[tzik, 2016/03/02 13:09:52]

Each clang, gcc and cl.exe behave differently around the symbol visibility and
explicit template instantiation...
I don't know what the best practice is.

[dcheng, 2016/03/02 17:04:56]

I /think/ you should only need this on the declaration. But I'm not 100% sure.

[Nico, 2016/03/09 03:24:53]

See also ipc/export_template.h for some text (and horrible hacks) about exported
templates.

+    : bind_state_(std::move(c.bind_state_)),
+      polymorphic_invoke_(c.polymorphic_invoke_) {
+  c.polymorphic_invoke_ = nullptr;
+}
+
+BASE_EXPORT CallbackBase<CopyMode::MoveOnly>&
+CallbackBase<CopyMode::MoveOnly>::operator=(CallbackBase&& c) {
+  bind_state_ = std::move(c.bind_state_);
+  polymorphic_invoke_ = c.polymorphic_invoke_;
+  c.polymorphic_invoke_ = nullptr;
+  return *this;
+}
 
-void CallbackBase::Reset() {
-  polymorphic_invoke_ = NULL;
+BASE_EXPORT void CallbackBase<CopyMode::MoveOnly>::Reset() {
+  polymorphic_invoke_ = nullptr;
   // NULL the bind_state_ last, since it may be holding the last ref to whatever
   // object owns us, and we may be deleted after that.
-  bind_state_ = NULL;
+  bind_state_ = nullptr;
 }
 
-bool CallbackBase::Equals(const CallbackBase& other) const {
+BASE_EXPORT bool CallbackBase<CopyMode::MoveOnly>::EqualsInternal(
+    const CallbackBase& other) const {
   return bind_state_.get() == other.bind_state_.get() &&
          polymorphic_invoke_ == other.polymorphic_invoke_;
 }
 
-CallbackBase::CallbackBase(BindStateBase* bind_state)
-    : bind_state_(bind_state),
-      polymorphic_invoke_(NULL) {
+BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::CallbackBase(
+    BindStateBase* bind_state)
+    : bind_state_(bind_state) {
   DCHECK(!bind_state_.get() || bind_state_->ref_count_ == 1);
 }
 
-CallbackBase::~CallbackBase() {
+BASE_EXPORT CallbackBase<CopyMode::MoveOnly>::~CallbackBase() {}
+
+BASE_EXPORT CallbackBase<CopyMode::Copyable>::CallbackBase(CallbackBase&& c)
+    : CallbackBase<CopyMode::MoveOnly>(std::move(c)) {}
+
+BASE_EXPORT CallbackBase<CopyMode::Copyable>&
+CallbackBase<CopyMode::Copyable>::operator=(CallbackBase&& c) {
+  *static_cast<CallbackBase<CopyMode::MoveOnly>*>(this) = std::move(c);
+  return *this;
+}

[dcheng, 2016/03/02 08:02:36]

Can we default the move operators?

It seems like this should work:
$ cat test.cc 
#include <iostream>
#include <utility>

struct MoveOnly {
  explicit MoveOnly(int x) : x(x) { }
  MoveOnly(MoveOnly&& rhs) : x(0) {
    using std::swap;
    swap(x, rhs.x);
  }
  MoveOnly& operator=(MoveOnly&& rhs) {
    using std::swap;
    swap(x, rhs.x);
  }
  int x;
};

struct Copyable : MoveOnly {
  explicit Copyable(int x) : MoveOnly(x) {
  }
  Copyable(const Copyable& rhs) : Copyable(x) {
  }
  Copyable(Copyable&&) = default;
  Copyable& operator=(Copyable&&) = default;
};

int main() {
  Copyable a(2);
  Copyable b(3);
  std::cout << "a: " << a.x << ", b: " << b.x << "\n";
  a = std::move(b);
  std::cout << "a: " << a.x << ", b: " << b.x << "\n";
  Copyable c(std::move(a));
  std::cout << "a: " << a.x << ", c: " << c.x << "\n";
  return 0;
}

$ ./a.out
a: 2, b: 3
a: 3, b: 2
a: 0, c: 3

Also minor nit that these should be in the same order as they are in the header.

[tzik, 2016/03/02 08:13:14]

We are still using MSVC2013, which does not support defaulted move constructor.
MSVC2015 seems to support it and hopefully the compiler switch will happend this
week.

+
+BASE_EXPORT CallbackBase<CopyMode::Copyable>::CallbackBase(
+    const CallbackBase& c)
+    : CallbackBase<CopyMode::MoveOnly>(nullptr) {
+  bind_state_ = c.bind_state_;
+  polymorphic_invoke_ = c.polymorphic_invoke_;
 }
 
+BASE_EXPORT CallbackBase<CopyMode::Copyable>&
+CallbackBase<CopyMode::Copyable>::operator=(const CallbackBase& c) {
+  bind_state_ = c.bind_state_;
+  polymorphic_invoke_ = c.polymorphic_invoke_;
+  return *this;
+}
+
+template class CallbackBase<CopyMode::MoveOnly>;
+template class CallbackBase<CopyMode::Copyable>;
+
 }  // namespace internal
 }  // namespace base