diff options
author | Kjell Ahlstedt <kjell.ahlstedt@bredband.net> | 2015-10-21 18:43:56 +0200 |
---|---|---|
committer | Kjell Ahlstedt <kjell.ahlstedt@bredband.net> | 2015-10-21 18:43:56 +0200 |
commit | cabc88f6c2e25432a8e6245c5af114ebb0de04b8 (patch) | |
tree | 95f1e0ca97aa8ec2724523599dfe1f83bba04f77 | |
parent | 02339149a28e26151402076e8cf1abd00f7dd92b (diff) | |
download | sigc++-cabc88f6c2e25432a8e6245c5af114ebb0de04b8.tar.gz |
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.
-rw-r--r-- | sigc++/functors/macros/slot.h.m4 | 37 | ||||
-rw-r--r-- | sigc++/functors/slot_base.cc | 72 | ||||
-rw-r--r-- | sigc++/functors/slot_base.h | 11 | ||||
-rw-r--r-- | sigc++/signal_base.cc | 15 | ||||
-rw-r--r-- | tests/test_signal_move.cc | 6 | ||||
-rw-r--r-- | 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)"); |