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
scoombs01 · Apr 13, 2018 · 5b1ab44 · 5b1ab44
1 parent 1975818
commit 5b1ab44
Show file tree

Hide file tree

Showing 2 changed files with 102 additions and 16 deletions.
diff --git 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<zmq::poller_t>::value, "poller_t should not be copy-constructible");
+static_assert(!std::is_copy_assignable<zmq::poller_t>::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<zmq::poller_t> 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<zmq::poller_t> 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<zmq::poller_t> 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<zmq::poller_t> 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 @@ class loopback_ip4_binder
     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
@@ -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(short)>;
 
-        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<void(void)> &handler)
+        {
+		    add (socket, events, [&handler](short) { handler(); });
+		}
+
+        void add (zmq::socket_t &socket, short events, handler_t handler)
         {
-            if (0 == zmq_poller_add (poller_ptr, socket.ptr, handler ? &handler : NULL, events)) {
+		    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<zmq_poller_event_t> poller_events;
+		std::map<socket_t*, handler_t> handlers;
     };  // class poller_t
 #endif //  defined(ZMQ_BUILD_DRAFT_API) && defined(ZMQ_CPP11) && defined(ZMQ_HAVE_POLLER)