NoAvailableAlias / nano-signal-slot

Pure C++17 Signals and Slots
MIT License
407 stars 60 forks source link

Explicitly default/delete copy and move constructors for signal and observer types #33

Closed captaincrutches closed 2 years ago

captaincrutches commented 2 years ago

This applies the Rule of Five to signal and observer types, explicitly specifying their default destructors, copiers, and movers.

Motivation:

I had a class derived from Observer<> which I wanted to be owned by movable objects.
Objects of that class could not be moved, and were not even MoveInsertable for e.g. vector::emplace_back.
This turned out to be because the move constructor on Observer was not explicitly defined, and adding a default to Observer allowed my type to be moved.

While I was at it, I also went ahead and explicitly defined defaulted/deleted move/copy operators for the policy classes.

Update Spin_Mutex contains only an atomic_bool, so it can be trivially lock-free copied and moved by simply copying the bool value.
This means that all policies, ST and TS alike, are both movable and copyable. While this opens up the possibility of a non-deleted copy for Observer, I'm keeping it non-copyable because that seems like a bigger change of behavior than is in scope here.

TL;DR - given this class

class TestObserver : public Nano::Observer<>
{
public:
    TestObserver() = default;
    ~TestObserver() = default;

    TestObserver(const TestObserver&) = default;
    TestObserver& operator=(const TestObserver&) = default;

    TestObserver(TestObserver&&) = default;
    TestObserver& operator=(TestObserver&&) = default;
};

this change makes it so that a TestObserver can be moved.

NoAvailableAlias commented 2 years ago

Hey thanks for the PR and sorry for the late response!

The changes look good, however, can you uncomment and verify the Test_Signal_Move test methods all pass? These can be located in the 4 Policy tests. Once verified, can you also update the tests/readme.md file to reflect the now supported test cases?

Edit: If you aren't on windows / don't have visual studio to run the tests I can checkout cc-move-constructors and take a look.

I had tried solving moving previously without any additional class property but if it can't be helped it can't be helped.

Thanks again for the PR!

captaincrutches commented 2 years ago

Thanks for the pointer - fired up my windows laptop and gave the tests a look.

I did have to make one fix - calling insert when we already had a lock was no good, so I pulled the actual insertion part into insertInternal and called that instead, so each caller gets its own lock beforehand.

Move tests now pass, and I've updated the readme.

captaincrutches commented 2 years ago

I had tried solving moving previously without any additional class property but if it can't be helped it can't be helped.

In fact, looking at the code again, it seems to me that we don't actually need movedFrom as a new class property at all!

Since during move_connections_from, we're already doing to other what disconnect_all does, and then clearing other->connections... then the disconnect_all in the destructor is a noop on the moved-from object, so we don't need to worry about whether we've been moved from at all!

Correct me if I'm wrong, but I believe this solves that problem. Tests still pass.

NoAvailableAlias commented 2 years ago

Thanks, I believe you are correct in regards to not needing movedFrom after all.

I've checked out the cc-move-constructors branch and have one nitpick change and one slightly more involved. First, can Spin_Mutex be changed to be a final class so that virtual can be removed from the destructor.

Second, I believe a new optionally implemented policy method needs created in order to simulate std::scoped_lock functionality in order to lock both the lock and otherLock in move_connections_from in a "safe" manner.

I'll be trying out some changes in policies and will report back here for now.

NoAvailableAlias commented 2 years ago

I've finally been able to take a look into adding a "scoped_lock" function and while it is doable, it requires try_lock() to be added to Spin_Mutex and to the two threadsafe policies. Additionally, the optional scoped_lock function itself needs added to all policies / called in move_connections_from.

I'm going to accept this PR into a new dev branch and add the above changes / normalization before release.