[libsigc++2] slot and signal: Fix move constructors and move assignments

[Kjell Ahlstedt <kjellahl src gnome org>]

commit cabc88f6c2e25432a8e6245c5af114ebb0de04b8
Author: Kjell Ahlstedt <kjell ahlstedt bredband net>
Date:   Wed Oct 21 18:43:56 2015 +0200

    slot and signal: Fix move constructors and move assignments
    
    * sigc++/functors/macros/slot.h.m4: Add documentation.
    * sigc++/functors/slot_base.[h|cc]: Fix the move operators of slot_base.
    Don't move a connected slot.
    * sigc++/signal_base.cc: Fix the move assignment of signal_base.
    * tests/test_signal_move.cc:
    * tests/test_slot_move.cc: Really test move assignment. Bug #756484.

 sigc++/functors/macros/slot.h.m4 |   37 ++++++++++++++-----
 sigc++/functors/slot_base.cc     |   72 +++++++++++++++++++++++++++++++------
 sigc++/functors/slot_base.h      |   11 +++++-
 sigc++/signal_base.cc            |   15 +++++++-
 tests/test_signal_move.cc        |    6 ++-
 tests/test_slot_move.cc          |    6 ++-
 6 files changed, 118 insertions(+), 29 deletions(-)
---
diff --git a/sigc++/functors/macros/slot.h.m4 b/sigc++/functors/macros/slot.h.m4
index 22eeaa9..ff199d5 100644
--- a/sigc++/functors/macros/slot.h.m4
+++ b/sigc++/functors/macros/slot.h.m4
@@ -85,29 +85,41 @@ FOR(1, $1,[
       slot_base::rep_->call_ = internal::slot_call$1<LIST(T_functor, T_return, LOOP(T_arg%1, 
$1))>::address();
     }
 
+  /** Constructs a slot, copying an existing one.
+   * @param src The existing slot to copy.
+   */
   slot$1(const slot$1& src)
     : slot_base(src)
     {}
 
+  /** Constructs a slot, moving an existing one.
+   * If @p src is connected to a parent (e.g. a signal), it is copied, not moved.
+   * @param src The existing slot to move or copy.
+   */
   slot$1(slot$1&& src) noexcept
     : slot_base(std::move(src))
     {}
 
-  /** Overrides this slot making a copy from another slot.
+  /** Overrides this slot, making a copy from another slot.
    * @param src The slot from which to make a copy.
    * @return @p this.
    */
   slot$1& operator=(const slot$1& src)
-    {
-      slot_base::operator=(src);
-      return *this;
-    }
-
+  {
+    slot_base::operator=(src);
+    return *this;
+  }
+
+  /** Overrides this slot, making a move from another slot.
+   * If @p src is connected to a parent (e.g. a signal), it is copied, not moved.
+   * @param src The slot from which to move or copy.
+   * @return @p this.
+   */
   slot$1& operator=(slot$1&& src) noexcept
-    {
-      slot_base::operator=(std::move(src));
-      return *this;
-    }
+  {
+    slot_base::operator=(std::move(src));
+    return *this;
+  }
 };
 
 ])
@@ -169,6 +181,11 @@ public:
   slot(const T_functor& _A_func)
     : parent_type(_A_func) {}
 
+  // Without reinterpret_cast (or static_cast) parent_type(const T_functor& _A_func)
+  // is called instead of the copy constructor.
+  /** Constructs a slot, copying an existing one.
+   * @param src The existing slot to copy.
+   */
   slot(const slot& src)
     : parent_type(reinterpret_cast<const parent_type&>(src)) {}
 };
diff --git a/sigc++/functors/slot_base.cc b/sigc++/functors/slot_base.cc
index c15c213..7500d84 100644
--- a/sigc++/functors/slot_base.cc
+++ b/sigc++/functors/slot_base.cc
@@ -125,12 +125,35 @@ slot_base::slot_base(const slot_base& src)
 }
 
 slot_base::slot_base(slot_base&& src) noexcept
-: rep_(std::move(src.rep_)),
-  blocked_(std::move(src.blocked_))
+: rep_(nullptr),
+  blocked_(src.blocked_)
 {
-  //Wipe src:
-  src.rep_ = nullptr;
-  src.blocked_ = false;
+  if (src.rep_)
+  {
+    if (src.rep_->parent_)
+    {
+      // src is connected to a parent, e.g. a sigc::signal.
+      // Copy, don't move! See https://bugzilla.gnome.org/show_bug.cgi?id=756484
+
+      //Check call_ so we can ignore invalidated slots.
+      //Otherwise, destroyed bound reference parameters (whose destruction
+      //caused the slot's invalidation) may be used during dup().
+      if (src.rep_->call_)
+        rep_ = src.rep_->dup();
+      else
+        blocked_ = false; //Return the default invalid slot.
+    }
+    else
+    {
+      // src is not connected. Really move src.rep_.
+      src.rep_->notify_callbacks();
+      rep_ = src.rep_;
+
+      //Wipe src:
+      src.rep_ = nullptr;
+      src.blocked_ = false;
+    }
+  }
 }
 
 slot_base::~slot_base()
@@ -199,17 +222,42 @@ slot_base& slot_base::operator=(const slot_base& src)
 slot_base& slot_base::operator=(slot_base&& src) noexcept
 {
   if (src.rep_ == rep_)
+  {
+    blocked_ = src.blocked_;
     return *this;
- 
-  delete_rep_with_check();
+  }
 
-  rep_ = std::move(src.rep_);
-  blocked_ = std::move(src.blocked_);
+  if (src.empty())
+  {
+    delete_rep_with_check();
+    return *this;
+  }
 
-  //Wipe src:
-  src.rep_ = nullptr;
-  src.blocked_ = false;
+  blocked_ = src.blocked_;
+  internal::slot_rep* new_rep_ = nullptr;
+  if (src.rep_->parent_)
+  {
+    // src is connected to a parent, e.g. a sigc::signal.
+    // Copy, don't move! See https://bugzilla.gnome.org/show_bug.cgi?id=756484
+    new_rep_ = src.rep_->dup();
+  }
+  else
+  {
+    // src is not connected. Really move src.rep_.
+    src.rep_->notify_callbacks();
+    new_rep_ = src.rep_;
 
+    //Wipe src:
+    src.rep_ = nullptr;
+    src.blocked_ = false;
+  }
+
+  if (rep_) // Silently exchange the slot_rep.
+  {
+    new_rep_->set_parent(rep_->parent_, rep_->cleanup_);
+    delete rep_; // Calls destroy(), but does not call disconnect().
+  }
+  rep_ = new_rep_;
   return *this;
 }
 
diff --git a/sigc++/functors/slot_base.h b/sigc++/functors/slot_base.h
index 3185c60..c33733d 100644
--- a/sigc++/functors/slot_base.h
+++ b/sigc++/functors/slot_base.h
@@ -250,6 +250,10 @@ public:
    */
   slot_base(const slot_base& src);
 
+  /** Constructs a slot, moving an existing one.
+   * If @p src is connected to a parent (e.g. a signal), it is copied, not moved.
+   * @param src The existing slot to move or copy.
+   */
   slot_base(slot_base&& src) noexcept;
 
   ~slot_base();
@@ -321,12 +325,17 @@ public:
 //The Tru64 and Solaris Forte 5.5 compilers needs this operator=() to be public. I'm not sure why, or why it 
needs to be protected usually. murrayc.
 //See bug #168265. 
 //protected:
-  /** Overrides this slot making a copy from another slot.
+  /** Overrides this slot, making a copy from another slot.
    * @param src The slot from which to make a copy.
    * @return @p this.
    */
   slot_base& operator=(const slot_base& src);
 
+  /** Overrides this slot, making a move from another slot.
+   * If @p src is connected to a parent (e.g. a signal), it is copied, not moved.
+   * @param src The slot from which to move or copy.
+   * @return @p this.
+   */
   slot_base& operator=(slot_base&& src) noexcept;
 
 public: // public to avoid template friend declarations
diff --git a/sigc++/signal_base.cc b/sigc++/signal_base.cc
index c968216..64226b8 100644
--- a/sigc++/signal_base.cc
+++ b/sigc++/signal_base.cc
@@ -250,9 +250,20 @@ signal_base& signal_base::operator=(const signal_base& src)
 
 signal_base& signal_base::operator=(signal_base&& src) noexcept
 {
-  trackable::operator=(std::move(src));
-  impl_ = std::move(src.impl_);
+  if (src.impl_ == impl_) return *this;
+
+  if (impl_)
+  {
+    // Disconnect all slots before impl_ is deleted.
+    // TODO: Move the signal_impl::clear() call to ~signal_impl() when ABI can be broken.
+    if (impl_->ref_count_ == 1)
+      impl_->clear();
+
+    impl_->unreference();
+  }
 
+  src.notify_callbacks();
+  impl_ = src.impl_;
   src.impl_ = nullptr;
 
   return *this;
diff --git a/tests/test_signal_move.cc b/tests/test_signal_move.cc
index db60b56..73a7ed2 100644
--- a/tests/test_signal_move.cc
+++ b/tests/test_signal_move.cc
@@ -1,4 +1,3 @@
-// -*- c++ -*-
 /* Copyright 2015, The libsigc++ Development Team
  *  Assigned to public domain.  Use as you wish without restriction.
  */
@@ -38,11 +37,14 @@ int main(int argc, char* argv[])
 
   //Test the move constructor:
   sigc::signal<int, int> sig2(std::move(sig));
+  //sig(-2); Add when more move constructors have been added
   sig2(2);
   util->check_result(result_stream, "foo(int 2)");
 
   //Test the move assignment operator:
-  sigc::signal<int, int> sig3 = std::move(sig2);
+  sigc::signal<int, int> sig3;
+  sig3 = std::move(sig2);
+  //sig2(-3); Add when more move assignment operators have been added
   sig3(3);
   util->check_result(result_stream, "foo(int 3)");
 
diff --git a/tests/test_slot_move.cc b/tests/test_slot_move.cc
index 62896b9..df36d0b 100644
--- a/tests/test_slot_move.cc
+++ b/tests/test_slot_move.cc
@@ -1,4 +1,3 @@
-// -*- c++ -*-
 /* Copyright 2015, The libsigc++ Development Team
  *  Assigned to public domain.  Use as you wish without restriction.
  */
@@ -54,11 +53,14 @@ int main(int argc, char* argv[])
 
   // test move constructor:
   sigc::slot<void,int> s2(std::move(s1));
+  //s1(-2); Add when more move constructors have been added
   s2(2);
   util->check_result(result_stream, "foo(int 2)");
 
   // test move assignment:
-  sigc::slot<void,int> s3 = std::move(s2);
+  sigc::slot<void,int> s3;
+  s3 = std::move(s2);
+  //s2(-3); Add when more move assignment operators have been added
   s3(3);
   util->check_result(result_stream, "foo(int 3)");