> although they call notify_callbacks(), which is not noexcept, and may throw an > exception. Is that ok?

That would mean that std::terminate is called when an exception is thrown there. I don't know whether that's ok though.

Created attachment 313466 [details] [review]
patch: slot and signal: Fix move constructors and move assignments

This is what I believe should be done to the move operators.
In addition to the modifications listed in comment 0, I have removed "noexcept"
from slot's move operators. That's reasonable when they sometimes make a copy
instead of a move.

In text books move operations are simple. In libsigc++ they are not.
In text book examples an object, A, contains a pointer to a huge object, B.
B does not contain a pointer to A, neither directly nor indirectly via one or
more other objects. A move just comprises
  delete pb; // in assignment
  pb = src.pb;
  src.pb = nullptr;
In libsigc++ it's not that easy.

I just realized that I should split my patch into two: one that fixes existing
move operators and can be included in libsigc++ 2.6.x, and another one that
adds missing move operators, i.e. adds new API, and must wait until 2.7.x.

Created attachment 313819 [details] [review]
patch: slot and signal: Add missing move constructors and move assignments

I have pushed https://git.gnome.org/browse/libsigc++2/commit/?id=cabc88f6c2e25432a8e6245c5af114ebb0de04b8
with the fixes that don't add new API.

The attached patch contains added move operators. It can be pushed to the master
branch when the libsigc-2-6 branch is created. I can do it now, if someone wants
that new API now, otherwise it can wait a while.

(In reply to Kjell Ahlstedt from comment #0)
> I doubt that all move constructors and move assignment operators in libsigc++
> are correct.

Many thanks for reviewing all this. Sorry for not finding the time until now to respond.

> CC = copy constructor
> CA = copy assignment operator
> MC = move constructor
> MA = move assignment operator
> !! = need more thought and/or fix
> 
> - trackable
>   User-defined CC, CA, MC, MA. Ok after the fix of bug 755393.
>   !! MC and MA are noexcept, although they call notify_callbacks(),
>   which is not noexcept, and may throw an exception. Is that ok?

No, it's not OK. Maybe we can make notify_callbacks() and any callbacks noexcept?
 
> - slot_rep
>   Compiler-generated CC, CA, MC, MA.
>   !! Should be deleted. These operators are never used.

Yes. Let's use = delete there.
I'm fine with doing this in the current branch. It's not going to be very disruptive, I guess.

> - typed_slot_rep
>   User-defined CC. Called by dup(). Calls a user-defined slot_rep constructor
>   (not CC).
>   Compiler-generated CA.
>   No MC, MA, because there is a user-defined CC, and no user-defined MC, MA.
>   !! CA, MC, MA should be deleted.

Yes, likewise.
 
> - slot_base
>   User-defined CC, CA, MC, MA.
>   CC, CA ok.
>   !! MC not ok. Not reasonable to move a connected slot_base.
>      If src is connected (src.rep->parent_ != nullptr), then do like CC.
>      If src is not connected, then add a call to
> src.rep_->notify_callbacks().
>   !! MA not ok. Keep rep_->parent_ and rep_->cleanup_, like CA.
>      If src is connected (src.rep_->parent_ != nullptr), then do like CA.
>      If src is not connected, then add a call to
> src.rep_->notify_callbacks().

Whenever we have logic in the MC and MA, it really seems like something we should test.
 
> - slot1..slot7
>   User-defined CC, CA, MC, MA. Ok.
> 
> - slot
>   User-defined CC.
>   Compiler-generated CA.
>   No MC, MA, because there is a user-defined CC, and no user-defined MC, MA.
>   !! Add user-defined CA, MC, MA.

You've pushed these. Great.

> - signal_impl
>   Compiler-generated CC, CA, MC, MA.
>   !! Should be deleted. signal_impl is never copied or moved.

Yes, as for other needed "= delete" mentions above.

> - signal_base
>   User-defined CC, CA, MC, MA.
>   CC, CA, MC ok.
>   !! MA not ok. No self-assignment guard. Old impl_ must be handled like in
> CA
>      before it's reassigned. Shall it continue to call trackable::operator=()
>      or call src.notify_callbacks() instead? I recommend
> src.notify_callbacks(),
>      because CA does not call trackable::operator=(). MA should have the same
>      effect on the destination as CA has.
> 
> - signal1..signal7
>   User-defined CC.
>   Compiler-generated CA.
>   No MC, MA, because there is a user-defined CC, and no user-defined MC, MA.
>   !! Add user-defined CA, MC, MA.
> 
> - signal
>   User-defined CC.
>   Compiler-generated CA.
>   No MC, MA, because there is a user-defined CC, and no user-defined MC, MA.
>   !! Add user-defined CA, MC, MA.

You've done these. Thanks.
 
> The test programs libsigc++2/tests/test_signal_move.cc and test_slot.move.cc
> don't call any move operators. Neither signal<> nor slot<> has move
> operators.
> Both the tests that are said to test move constructors and those that are
> said
> to test move assignment actually call copy constructors.
> 
> Can we delete operators? Wouldn't it break API/ABI? No, not in this case.
> slot_rep, typed_slot_rep and signal_impl are declared in the sigc::internal
> namespace. They shall not be used directly by code outside libsigc++.
> 
> What about connected slots (slots with a parent)? It seems almost out of the
> question to move a connected slot_rep from one slot_base to another. First,
> it
> must be disconnected from its parent. The parent does not know or care about
> the
> slot_rep. The parent knows about the corresponding slot. It's up to the
> parent
> what to do when a slot is disconnected. The parent can for example delete the
> slot, including the corresponding slot_rep. Usually a slot with a parent is
> owned by the parent. It's possible that there is a solution that makes it
> possible to move connected slots, but it's not worth the trouble.
> 
> 
> Comments, before I fix the move operators?

Let's take care of the easy stuff first.

(In reply to Murray Cumming from comment #5)
> Maybe we can make notify_callbacks() and any callbacks noexcept?

We can make more functions noexcept, but it can't be made totally consistent,
because we don't have complete control over the callbacks. Some callbacks may
execute code from any package that uses libsigc++.

slot_rep::notify() will throw an exception, if either the callback set with
slot_base::set_parent(), or the destruction of the stored functor throws
an exception.

We could for instance mark noexcept:
  trackable::notify_callbacks()
  trackable::~trackable()
  trackable_callback_list::~trackable_callback_list()
  slot_rep::notify()
  connection::notify()
  destroy_notify_struct::notify()  // Local class in slot_base.cc

but only the last two are unconditionally non-throwing. All the other functions
may invoke code outside libsigc++. Would anything be gained by marking them
noexcept, although we can't give a guarantee?

We can add a note to the description of slot_base::set_parent() and
trackable::add_destroy_notify_callback(), saying that the program may be
terminated if the callback function throws an exception.

>> - slot
>>   !! Add user-defined CA, MC, MA.

> You've pushed these. Great.

No, I haven't. That's in the patch in comment 4.

>> - signal1..signal7
>>   !! Add user-defined CA, MC, MA.
>> 
>> - signal
>>   !! Add user-defined CA, MC, MA.

> You've done these. Thanks.

Likewise, not pushed yet.

The patch in comment 4 also removes "noexcept" from slot's move constructor and
move assignment. Do you think that's OK? I think it is, when they sometimes make
a copy instead of a move.

I think it would be a bad idea to make functions noexcept that will execute an external callback. Is it not possible to use something like noexcept(noexcept(callback())) in these cases? (there is a noexcept *operator* in addition to the specifier, it's used to determine if an expression is noexcept: http://en.cppreference.com/w/cpp/language/noexcept)

Can you explain in more detail what you have in mind? I don't understand how
noexcept(noexcept(callback())) can be used in libsigc++.

Whether trackable::notify_callbacks() can throw an exception or not depends on
which callbacks are registered with calls to trackable::
add_destroy_notify_callback() and slot_base::set_parent(). It also depends on
which function objects are stored in slots. If the destruction of such a
function object throws an exception, that exception can propagate through
slot_rep::notify() and trackable::notify_callbacks().

Okay, so I had a look at the documentation. I should have done that before, sorry.

That said, I have an idea how a conditional noexcept would be possible, but it's a bit crazy, as it involves making trackable a template (with a boolean parameter for whether all of its callbacks are noexcept, defaulting to true). But I guess I can stop there as that would break everything in some way?

The easiest fix concerning trackable's move operators would be to remove
"noexcept" from them. Is that acceptable?

(In reply to Kjell Ahlstedt from comment #10)
> The easiest fix concerning trackable's move operators would be to remove
> "noexcept" from them. Is that acceptable?

Yes, sure.

In general, when we have a reason for something to be noexcept, or not, please leave a comment describing that reason. Then we won't have to decide again later.

Incidentally, is trackable::add_destroy_notify_callback() actually used or needed outside of libsigc++ and *mm. Maybe a future libsigc++ should try to hide that from the main public API.

I have added the = delete lines that you suggested. These seem to be simple enough to just do:

https://git.gnome.org/browse/libsigc++2/commit/?id=416fc2d6eaf3b79fbc224ad29804a2f7778477d9
https://git.gnome.org/browse/libsigc++2/commit/?id=043cfa5ca2cac2bc45facd0cea0a4c500a47e498
https://git.gnome.org/browse/libsigc++2/commit/?id=d14750e547387ab245af2d489c396fa699ab4263

Created attachment 314991 [details] [review]
patch: trackable, slot, signal: Remove noexcept specifications

Now it remains to remove noexcept specifications and add missing move operators.
This patch removes noexcept. Can it be implemented in libsigc++ 2.6.x?
Or is it an API break that must wait for 2.7.x?

Created attachment 314992 [details] [review]
patch: slot and signal: Add missing move constructors and move assignments

This patch adds missing move operators. I suppose it must wait for 2.7.x.

There are methods in libsigc++ that don't throw exceptions.
If noexcept specifications are important, I can find some of these methods and
add some noexcept specifications. Would such added specifications be an API break
that must wait for 2.7.x?

(In reply to Murray Cumming from comment #11)
> Incidentally, is trackable::add_destroy_notify_callback() actually used or
> needed outside of libsigc++ and *mm. Maybe a future libsigc++ should try to
> hide that from the main public API.

Perhaps no one outside libsigc++ needs trackable::add_destroy_notify_callback().
But even if we hide it, that won't make trackable::notify_callbacks() noexcept.
The slot_rep::notify() callback calls the callback set by slot_base::set_parent()
and it calls the destructor of the stored functor, which can be anything.
A throwing destructor is a bad thing, but libsigc++ has no control of what such
a destructor might do.

(In reply to Kjell Ahlstedt from comment #13)
> This patch removes noexcept. Can it be implemented in libsigc++ 2.6.x?

Yes, I think that should be fine. I don't think noexcept is part of the ABI. And this would not be a disruptive API change.

(In reply to Kjell Ahlstedt from comment #14)
> Created attachment 314992 [details] [review] [review]
> patch: slot and signal: Add missing move constructors and move assignments
> 
> This patch adds missing move operators. I suppose it must wait for 2.7.x.

Please just go ahead with it now. I don't think enough people can already be using libsigc++ 2.6 for the ABI addition to be disruptive. I think it's worth it to get 2.6.x generally correct, to simplify things.

> There are methods in libsigc++ that don't throw exceptions.
> If noexcept specifications are important,

I tried to add them where possible for move operations, but that allows standard containers to use them. I think they can generally improve performance by removing the need for the compiler to add exception handling. It seems wise to use them where we can, though it's easy to get it wrong.

> I can find some of these methods
> and
> add some noexcept specifications. Would such added specifications be an API
> break
> that must wait for 2.7.x?

I think that would be fine. I don't think it would affect application developers.

Thanks as usual.

> I tried to add them where possible for move operations, but that allows
> standard containers to use them.

I mean "because that allows", rather than "but that allows".

I have pushed the two patches.

I'll add some "noexcept" later. That can be done without reference to a bug
report. I close this bug now.

> I tried to add them where possible for move operations, because that allows
> standard containers to use them.

Must move operations be declared noexcept for STL containers to use them?
The description of std::vector::push_back() at
http://en.cppreference.com/w/cpp/container/vector/push_back says

  If T's move constructor is not noexcept and T is not CopyInsertable into
  *this, vector will use the throwing move constructor. If it throws, the
  guarantee is waived and the effects are unspecified.

If this is typical of the STL containers, they can use all move operators, but
if a move operator does throw an exception, the program can crash in an
unspecified way.

(In reply to Kjell Ahlstedt from comment #19)
> Must move operations be declared noexcept for STL containers to use them?
[snip]

Maybe something has changed in the standard? "Effective Modern C++" says "All these functions replace calls to copy operations in C++98 with calls to move operations in C++11 only if the move operations are known to not emit exceptions"
This link seems to get you near to that text in the book:
http://tinyurl.com/o9pgptp
Maybe it's wrong now.

There's some earlier push_back documentation here, and a snippet of the standard:
http://stackoverflow.com/questions/11488490/vectorpush-back-insists-on-using-copy-constructor-though-a-move-constructor-is

I think a throwing move constructor (and hence "undefined" crashing) will only be used if the copy constructor is not available. So it really would be nice to have noexcept move operations where possible, so the standard containers can use them in normal cases. However, I understand that we can't do it in this case.

> Maybe something has changed in the standard?

Maybe it has not. The text I linked to in comment 19 says that a move constructor
which is not noexcept is used _if_the_object_is_not_CopyInsertable_. I did not
pay enough attention to that clause at first. It's what your links also say.
They also explain why a possibly throwing copy constructor is preferred to a
possibly throwing move constructor.

This would mean that gtkmm widgets, which are not copyable, can be moved to
an STL container, such as a std::vector, whether or not their move constructors
are declared noexcept.

I'll make a new tarball release soon, if you think that's OK.

Created attachment 315313 [details] [review]
patch: Add a moving signal::connect() overload

I've been experimenting with a moving version of signal::connect(), and I was
just about to ask if it would fit in libsigc++ 2.6.2, or if it shall wait
until 2.7.x.


If there is a moving signal::connect() overload, i.e. one that takes an
rvalue reference of a slot, then slot's move constructor is used in many
situations where its copy constructor is now used.

I have added moving signal#::connect() and moving overloads of some other
connect() and insert() methods, and run some tests.

The following test results show the number of calls to slot_base's move
operators when libsigc++'s test suite is executed. Copy/move insert() shows
the number of calls to signal_impl::insert(), which is called from
signal#::connect().

Without moving versions of signal::connect() and friends

Copy insert()                33
Move insert()                 0  does not exist
Copy constructor             45
Copy assignment operator      2
Move constructor             31
  of which really moving         1
Move assignment operator     24
  of which really moving        22

With moving versions of signal::connect() and friends

Copy insert()                 0
Move insert()                33
Copy constructor             10
Copy assignment operator      2
Move constructor             64
  of which really moving        34
Move assignment operator     24
  of which really moving        22

"Really moving" means that a call to a move operator has actually moved a
slot_rep instance from one slot to another. Some calls to a move operator does
not really move, in most cases because the slot is empty, and there is nothing
to move. Each time a signal is emitted, an empty slot is inserted at the end of
the slot list, and removed after the emission.

The test suite is hardly an example of typical use of libsigc++, but these
test results still show that it's worthwhile to add moving connect() and
insert() methods. I'm sure it would be even more worthwhile to do something
similar in glibmm.

Why is slot's move constructor used although it may throw exceptions and
there is a corresponding copy constructor? We have just learned that STL avoids
such move constructors. Or haven't we? I don't think we have. We have learned
that std::vector avoids such move constructors. But a signal stores slots in a
std::list, and a list does not have the same reason to avoid exceptions as a
vector has. When an element is added to or removed from a list, no other
element is reallocated. That is in fact an essential characteristic of a list,
which is (I suppose) the reason why it was chosen as the container of connected
slots in a sigc::signal. Much of the code in sigc::signal and sigc::connection
would not work if connected slots were ever reallocated.

Great. It sounds worthwhile.

But maybe we should delay this one until 2.7, just to reduce some risk?

An obvious risk is this: It's new API and ABI. If it's included in 2.6.2, a
program built with 2.6.2 may not be able to run with an object library
file of version 2.6.1. or 2.6.0.

I don't think the same risk exists with the fixes in sigc::slot and sigc::signal,
because the added move operators are defined in template classes in header files.

Suggested way forward:
You release libsigc++ 2.6.2 whenever you like.
When that's done, I can make a libsigc-2-6 branch and add the new
signal::connect() method to the master branch. I'll just change the @newin
directives in my patch from 2.6.2 to 2.8.

I have pushed a patch that adds a moving signal::connect() overload. It's
identical to the patch in comment 24 except for the @newin directives.
https://git.gnome.org/browse/libsigc++2/commit/?id=31132598511b2bfaa6671acbf6c2ed5a5eb151c6