diff --git a/lib/src/dexode/eventbus/Listener.hpp b/lib/src/dexode/eventbus/Listener.hpp index ac646cf..0fb92d7 100644 --- a/lib/src/dexode/eventbus/Listener.hpp +++ b/lib/src/dexode/eventbus/Listener.hpp @@ -29,12 +29,9 @@ public: } Listener(const Listener& other) = delete; - - Listener(Listener&& other) noexcept - : _id(other._id) // we don't have to reset listener ID as _bus is moved and we won't call - // unlistenAll - , _bus(std::move(other._bus)) - {} + // To see why move is disabled search for tag: FORBID_MOVE_LISTENER in tests + // Long story short, what if we capture 'this' in lambda during registering listener in ctor + Listener(Listener&& other) = delete; ~Listener() { @@ -45,24 +42,8 @@ public: } Listener& operator=(const Listener& other) = delete; - - Listener& operator=(Listener&& other) noexcept - { - if(this == &other) - { - return *this; - } - - if(_bus != nullptr) - { - unlistenAll(); // remove previous - } - // we don't have reset listener ID as bus is moved and we won't call unlistenAll - _id = other._id; - _bus = std::move(other._bus); - - return *this; - } + // To see why move is disabled search for tag: FORBID_MOVE_LISTENER in tests + Listener& operator=(Listener&& other) = delete; template constexpr void listen(std::function&& callback) @@ -127,6 +108,28 @@ public: internal::ListenerAttorney::unlisten(*_bus, _id, internal::event_id()); } + // We want more explicit move so user knows what is going on + void transfer(Listener&& from) + { + if(this == &from) + { + throw std::runtime_error("Self transfer not allowed"); + } + + if(_bus != nullptr) + { + unlistenAll(); // remove previous + } + // we don't have to reset listener ID as bus is moved and we won't call unlistenAll + _id = from._id; + _bus = std::move(from._bus); + } + + const std::shared_ptr& getBus() const + { + return _bus; + } + private: std::uint32_t _id = 0; std::shared_ptr _bus = nullptr; diff --git a/test/unit/src/dexode/eventbus/test/SuiteListener.cpp b/test/unit/src/dexode/eventbus/test/SuiteListener.cpp index 973ba17..deb7afa 100644 --- a/test/unit/src/dexode/eventbus/test/SuiteListener.cpp +++ b/test/unit/src/dexode/eventbus/test/SuiteListener.cpp @@ -1,3 +1,5 @@ +#include + #include #include "dexode/EventBus.hpp" @@ -46,7 +48,7 @@ TEST_CASE("Should unlisten all events When listener instance is overriden", "[Ev REQUIRE(bus.process() == 1); REQUIRE(callCount == 1); - listener = Listener{}; + listener.transfer(Listener{}); bus.postpone(event::Value{2}); REQUIRE(bus.process() == 1); @@ -90,7 +92,7 @@ TEST_CASE("Should keep listeners When listener is moved", "[EventBus][Listener]" REQUIRE(bus->process() == 1); REQUIRE(callCount == 1); - transferOne = std::move(listener); + transferOne.transfer(std::move(listener)); } bus->postpone(event::Value{3}); @@ -231,4 +233,68 @@ TEST_CASE("Should compile", "[EventBus][Listener]") } } +class TestClazz +{ +public: + static int counter; + + TestClazz(int id, const std::shared_ptr& bus) + : _id{id} + , _listener{bus} + { + registerListener(); + } + + TestClazz(TestClazz&& other) + : _id{other._id} + , _listener{other._listener.getBus()} + { + // We need to register again + registerListener(); + } + + ~TestClazz() + { + _id = 0; + } + +private: + int _id = 0; + EventBus::Listener _listener; + + void registerListener() + { + _listener.listen([this](const event::Value& event) { + if(_id == 1) + { + ++counter; + } + }); + } +}; + +int TestClazz::counter = 0; + +TEST_CASE("Should not allow for mistake with move ctor", "[EventBus][Listener]") +{ + /** + * Test case TAG: FORBID_MOVE_LISTENER + * + * This case is little bit complicated. + * We can't move EventBus::Listener as it capture 'this' in ctor so whenever we would use it it + * would lead to UB. + */ + std::shared_ptr bus = std::make_shared(); + + std::vector vector; + vector.emplace_back(1, bus); + vector.emplace_back(2, bus); + vector.emplace_back(3, bus); + + bus->postpone(event::Value{100}); + bus->process(); + + REQUIRE(TestClazz::counter == 1); +} + } // namespace dexode::eventbus::test