-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add move construction and assignment to signal #19
Conversation
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 |
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. |
9c0b602
to
df7cbc0
Compare
gone |
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 ( |
Good catch, I'll take a look at that |
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
This replaces
This takes the shared pointer referenced by |
In case it's relevant I'm using g++ 7.3.0, which compiler are you using for local tests? |
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 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 |
No description provided.