From 5b1ab442644e276f6eb14bb0b025ad477112553b Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Tue, 10 Apr 2018 18:35:58 +0200 Subject: [PATCH] Problem: insufficient tests for poller_t, bad usability since caller of add must store function object Solution: added tests, added size() method, added add deprecated overload with former signature, changed new add signature to accept handler by value --- tests/poller.cpp | 82 ++++++++++++++++++++++++++++++++++++++++-------- zmq.hpp | 36 +++++++++++++++++++-- 2 files changed, 102 insertions(+), 16 deletions(-) diff --git a/tests/poller.cpp b/tests/poller.cpp index 871c98e..7fd5a6d 100644 --- a/tests/poller.cpp +++ b/tests/poller.cpp @@ -8,19 +8,62 @@ TEST(poller, create_destroy) { zmq::poller_t poller; + ASSERT_EQ(0u, poller.size ()); } -TEST(poller, move_contruct) +static_assert(!std::is_copy_constructible::value, "poller_t should not be copy-constructible"); +static_assert(!std::is_copy_assignable::value, "poller_t should not be copy-assignable"); + +TEST(poller, move_construct_empty) { - zmq::poller_t poller; - zmq::poller_t poller_move {std::move (poller)}; + std::unique_ptr a{new zmq::poller_t}; + zmq::poller_t b = std::move(*a); + + ASSERT_EQ(0u, a->size ()); + a.reset (); + ASSERT_EQ(0u, b.size ()); } -TEST(poller, move_assign) +TEST(poller, move_assign_empty) { - zmq::poller_t poller; - zmq::poller_t poller_assign; - poller_assign = std::move (poller); + std::unique_ptr a{new zmq::poller_t}; + zmq::poller_t b; + + b = std::move(*a); + + ASSERT_EQ(0u, a->size ()); + a.reset (); + ASSERT_EQ(0u, b.size ()); +} + +TEST(poller, move_construct_non_empty) +{ + zmq::context_t context; + zmq::socket_t socket{context, zmq::socket_type::router}; + + std::unique_ptr a{new zmq::poller_t}; + a->add(socket, ZMQ_POLLIN, [](short) {}); + zmq::poller_t b = std::move(*a); + + ASSERT_EQ(0u, a->size ()); + a.reset (); + ASSERT_EQ(1u, b.size ()); +} + +TEST(poller, move_assign_non_empty) +{ + zmq::context_t context; + zmq::socket_t socket{context, zmq::socket_type::router}; + + std::unique_ptr a{new zmq::poller_t}; + a->add(socket, ZMQ_POLLIN, [](short) {}); + zmq::poller_t b; + + b = std::move(*a); + + ASSERT_EQ(0u, a->size ()); + a.reset (); + ASSERT_EQ(1u, b.size ()); } TEST(poller, add_handler) @@ -49,12 +92,14 @@ TEST(poller, add_handler_twice_throws) zmq::poller_t poller; zmq::poller_t::handler_t handler; poller.add(socket, ZMQ_POLLIN, handler); + /// \todo the actual error code should be checked ASSERT_THROW(poller.add(socket, ZMQ_POLLIN, handler), zmq::error_t); } TEST(poller, wait_with_no_handlers_throws) { zmq::poller_t poller; + /// \todo the actual error code should be checked ASSERT_THROW(poller.wait(std::chrono::milliseconds{10}), zmq::error_t); } @@ -63,16 +108,26 @@ TEST(poller, remove_unregistered_throws) zmq::context_t context; zmq::socket_t socket{context, zmq::socket_type::router}; zmq::poller_t poller; + /// \todo the actual error code should be checked ASSERT_THROW(poller.remove(socket), zmq::error_t); } -TEST(poller, remove_registered) +/// \todo this should lead to an exception instead +TEST(poller, remove_registered_empty) { zmq::context_t context; zmq::socket_t socket{context, zmq::socket_type::router}; zmq::poller_t poller; - zmq::poller_t::handler_t handler; - poller.add(socket, ZMQ_POLLIN, handler); + poller.add(socket, ZMQ_POLLIN, zmq::poller_t::handler_t{}); + ASSERT_NO_THROW(poller.remove(socket)); +} + +TEST(poller, remove_registered_non_empty) +{ + zmq::context_t context; + zmq::socket_t socket{context, zmq::socket_type::router}; + zmq::poller_t poller; + poller.add(socket, ZMQ_POLLIN, [](short) {}); ASSERT_NO_THROW(poller.remove(socket)); } @@ -85,7 +140,7 @@ public: std::string endpoint() { return endpoint_; } private: // Helper function used in constructor - // as Gtest allows only void returning functions + // as Gtest allows ASSERT_* only in void returning functions // and constructor/destructor are not. void bind(zmq::socket_t &socket) { @@ -112,7 +167,7 @@ TEST(poller, poll_basic) zmq::socket_t sink{context, zmq::socket_type::pull}; ASSERT_NO_THROW(sink.connect(endpoint)); - std::string message = "H"; + const std::string message = "H"; // TODO: send overload for string would be handy. ASSERT_NO_THROW(vent.send(std::begin(message), std::end(message))); @@ -128,10 +183,11 @@ TEST(poller, poll_basic) ASSERT_TRUE(message_received); } +/// \todo this contains multiple test cases that should be split up TEST(poller, client_server) { zmq::context_t context; - std::string send_msg = "Hi"; + const std::string send_msg = "Hi"; // Setup server zmq::socket_t server{context, zmq::socket_type::router}; diff --git a/zmq.hpp b/zmq.hpp index e6d293e..984dbe6 100644 --- a/zmq.hpp +++ b/zmq.hpp @@ -26,6 +26,14 @@ #ifndef __ZMQ_HPP_INCLUDED__ #define __ZMQ_HPP_INCLUDED__ +#if (__cplusplus >= 201402L) + #define ZMQ_DEPRECATED(msg) [[deprecated(msg)]] +#elif defined(_MSC_VER) + #define ZMQ_DEPRECATED(msg) __declspec(deprecated(msg)) +#elif defined(__GNUC__) + #define ZMQ_DEPRECATED(msg) __attribute__((deprecated(msg))) +#endif + #if (__cplusplus >= 201103L) #define ZMQ_CPP11 #define ZMQ_NOTHROW noexcept @@ -1022,9 +1030,21 @@ namespace zmq using handler_t = std::function; - void add (zmq::socket_t &socket, short events, handler_t &handler) + ZMQ_DEPRECATED("from 4.3.0, use overload accepting handler_t instead") + void add (zmq::socket_t &socket, short events, std::function &handler) { - if (0 == zmq_poller_add (poller_ptr, socket.ptr, handler ? &handler : NULL, events)) { + add (socket, events, [&handler](short) { handler(); }); + } + + void add (zmq::socket_t &socket, short events, handler_t handler) + { + handler_t *handler_ptr = nullptr; + /// \todo is it sensible to allow handler to be empty? doesn't this lead to an error when the event is signalled? (perhaps it should already lead to an error in zmq_poller_add then) + if (handler) { + auto emplace_res = handlers.emplace(&socket, std::move(handler)); + handler_ptr = &emplace_res.first->second; + } + if (0 == zmq_poller_add (poller_ptr, socket.ptr, handler_ptr, events)) { poller_events.emplace_back (zmq_poller_event_t ()); return; } @@ -1034,7 +1054,11 @@ namespace zmq void remove (zmq::socket_t &socket) { if (0 == zmq_poller_remove (poller_ptr, socket.ptr)) { - poller_events.pop_back (); + auto it = handlers.find (&socket); + if (it != handlers.end ()) { /// \todo this may only be false if handler was empty on add + handlers.erase (it); + } + poller_events.pop_back (); return; } throw error_t (); @@ -1060,10 +1084,16 @@ namespace zmq throw error_t (); } + + size_t size () + { + return poller_events.size(); + } private: void *poller_ptr; std::vector poller_events; + std::map handlers; }; // class poller_t #endif // defined(ZMQ_BUILD_DRAFT_API) && defined(ZMQ_CPP11) && defined(ZMQ_HAVE_POLLER)