Add move construction and assignment to signal

[oliverdaniell]

It looks like my formatting style is conflicting with yours (tabs vs spaces) do you have a style guide or auto formatter settings that I can use? I'll add the correct formatting and rebase

[fr00b0]

I see you fixed the formatting. I don't have a style guide or formatter settings currently. I'll look into adding that, but I predict it won't be very soon. Free time is lacking atm.

As I mentioned in your other PR, I'd prefer you remove the CMake stuff from this pull request.

[oliverdaniell]

I'd prefer you remove the CMake stuff from this pull request.

gone

[fr00b0]

Hello again,

I added this test locally:

TEST_CASE("Move assigning signals to with state", "[move_tests") {
    using output_signal = nod::signal<void(std::ostream&)>;
    output_signal signal_1;
    auto connection_1 = signal_1.connect(
        [](std::ostream& out){
        out << "1";
    });
    output_signal signal_2;
    auto connection_2 = signal_2.connect(
        [](std::ostream& out){
        out << "2";
    });

    signal_1 = std::move(signal_2);

    SECTION( "The moved-to instance has disconnected it's original connection" ) {
        REQUIRE(connection_1.connected() == false);
    }
    SECTION( "The moved-from instance has moved not disconnected its original connection, it is now owned by the moved-to instance" ) {
        REQUIRE(connection_2.connected() == true);
    }
    SECTION( "Triggering the moved-to signal has expected output" ) {
        std::stringstream ss;
        signal_1(ss);
        REQUIRE(ss.str() == "2");
    }
}

The test fails with your code, and what is happening is that the internal state for the two signals just swap. This leaves the connections that are connected to the original moved-to instance (signal_1 in this case) connected to the moved-from instance (signal_2). The connections will be disconnected when that instance is destructed. I find it would be more intuitive if the connections would disconnect immediately, since the instance is "invalid" for all operations other than destruction or re-initialization. At least that is my opinion, what do you think?

[oliverdaniell]

Good catch, I'll take a look at that

[oliverdaniell]

I can't reproduce the test failure locally, and it looks like it's passing on CI. As I understand it I do the following operations

381: _shared_disconnector = std::move(other._shared_disconnector);

This replaces signal_1._shared_disconnector with signal_2._shared_disconnector. The original signal_1._shared_disconnector referenced in connection_1 now has a reference count of 0 and signal_1.connected() == false.

382: *static_cast<disconnector*>(_shared_disconnector.get()) = _disconnector;

This takes the shared pointer referenced by connection_2 and sets the _disconnector to point to signal_1. Any connection that pointed to signal_2 now points to signal_1 as expected

[oliverdaniell]

In case it's relevant I'm using g++ 7.3.0, which compiler are you using for local tests?

[fr00b0]

I was using a rather old version of the msvc 2013 compiler for some reason. I was compiling on the command line on windows and had an old bat-file that used the wrong compiler.

I'm not able to reproduce this using the msvc 2017 compiler set to compile c++11.

After some digging, I did find a similar thing in the connection class, but that is my code and I'll be fixing that in a separate branch after this has been merged.

After compiling this with a proper compiler I think this looks good now, and will be merging this pull request.

Thank you for your contributions