Problem: inconsistent return values from mtrie_t::rm

Solution: Return an enum from rm instead of a bool, and adapt existing uses

src/generic_mtrie.hpp

@@ -44,12 +44,18 @@ template <typename T> class generic_mtrie_t
     typedef T value_t;
     typedef const unsigned char *prefix_t;
 
+    enum rm_result
+    {
+        not_found,
+        last_value_removed,
+        values_remain
+    };
+
     generic_mtrie_t ();
     ~generic_mtrie_t ();
 
-    //  Add key to the trie. Returns true if it's a new entry
-    //  rather than a duplicate (i.e. an entry with the same prefix
-    //  and the same or different value already exists).
+    //  Add key to the trie. Returns true iff no entry with the same prefix_
+    //  and size_ existed before.
     bool add (prefix_t prefix_, size_t size_, value_t *value_);
 
     //  Remove all entries with a specific value from the trie.
@@ -63,11 +69,9 @@ template <typename T> class generic_mtrie_t
              Arg arg_,
              bool call_on_uniq_);
 
-    //  Removes a specific entry from the trie. Return true if it was
-    //  actually removed rather than de-duplicated.
-    //  TODO this must be made clearer, and the case where the prefix/value
-    //  pair was not found must be specified
-    bool rm (prefix_t prefix_, size_t size_, value_t *value_);
+    //  Removes a specific entry from the trie.
+    //  Returns the result of the operation.
+    rm_result rm (prefix_t prefix_, size_t size_, value_t *value_);
 
     //  Calls a callback function for all matching entries, i.e. any node
     //  corresponding to data_ or a prefix of it. The arg_ argument
@@ -88,7 +92,7 @@ template <typename T> class generic_mtrie_t
                     void (*func_) (prefix_t data_, size_t size_, Arg arg_),
                     Arg arg_,
                     bool call_on_uniq_);
-    bool rm_helper (prefix_t prefix_, size_t size_, value_t *value_);
+    rm_result rm_helper (prefix_t prefix_, size_t size_, value_t *value_);
     bool is_redundant () const;
 
     typedef std::set<value_t *> pipes_t;

src/generic_mtrie_impl.hpp

@@ -294,47 +294,39 @@ void zmq::generic_mtrie_t<T>::rm_helper(value_t *pipe_,
 }
 
 template <typename T>
-bool zmq::generic_mtrie_t<T>::rm (prefix_t prefix_,
-                                  size_t size_,
-                                  value_t *pipe_)
+typename zmq::generic_mtrie_t<T>::rm_result
+zmq::generic_mtrie_t<T>::rm (prefix_t prefix_, size_t size_, value_t *pipe_)
 {
     return rm_helper (prefix_, size_, pipe_);
 }
 
 template <typename T>
-bool zmq::generic_mtrie_t<T>::rm_helper (prefix_t prefix_,
-                                         size_t size_,
-                                         value_t *pipe_)
+typename zmq::generic_mtrie_t<T>::rm_result zmq::generic_mtrie_t<T>::rm_helper (
+  prefix_t prefix_, size_t size_, value_t *pipe_)
 {
     if (!size_) {
-        // TODO pipes can only be NULL here, if we are at the top level, i.e.
-        // rm was already called with size_ == 0. This could be checked in rm,
-        // and here we could just have an assertion or naught
-        if (pipes) {
-            typename pipes_t::size_type erased = pipes->erase (pipe_);
-            // TODO this assertion prevents calling rm with a non-existent entry, but
-            // only if there is an entry with the same prefix but a different pipe;
-            // this appears inconsistent, see also unittest_mtrie. It might be
-            // removed (since pipes is a set, in cannot be more than 1), and an
-            // appropriate value must be returned.
+        if (!pipes)
+            return not_found;
+
+        typename pipes_t::size_type erased = pipes->erase (pipe_);
+        if (pipes->empty ()) {
             zmq_assert (erased == 1);
-            if (pipes->empty ()) {
-                LIBZMQ_DELETE (pipes);
-            }
+            LIBZMQ_DELETE (pipes);
+            return last_value_removed;
         }
-        return !pipes;
+        return (erased == 1) ? values_remain : not_found;
     }
 
     unsigned char c = *prefix_;
     if (!count || c < min || c >= min + count)
-        return false;
+        return not_found;
 
     generic_mtrie_t *next_node = count == 1 ? next.node : next.table[c - min];
 
     if (!next_node)
-        return false;
+        return not_found;
 
-    bool ret = next_node->rm_helper (prefix_ + 1, size_ - 1, pipe_);
+    rm_result ret = next_node->rm_helper (prefix_ + 1, size_ - 1, pipe_);
 
     if (next_node->is_redundant ()) {
         LIBZMQ_DELETE (next_node);

src/xpub.cpp

@@ -107,18 +107,23 @@ void zmq::xpub_t::xread_activated (pipe_t *pipe_)
                 pending_metadata.push_back (metadata);
                 pending_flags.push_back (0);
             } else {
-                bool unique;
-                if (*data == 0)
-                    unique = subscriptions.rm (data + 1, size - 1, pipe_);
-                else
-                    unique = subscriptions.add (data + 1, size - 1, pipe_);
-
-                //  If the (un)subscription is not a duplicate store it so that it can be
-                //  passed to the user on next recv call unless verbose mode is enabled
-                //  which makes to pass always these messages.
-                if (options.type == ZMQ_XPUB
-                    && (unique || (*data == 1 && verbose_subs)
-                        || (*data == 0 && verbose_unsubs && verbose_subs))) {
+                bool notify;
+                if (*data == 0) {
+                    mtrie_t::rm_result rm_result =
+                      subscriptions.rm (data + 1, size - 1, pipe_);
+                    //  TODO reconsider what to do if rm_result == mtrie_t::not_found
+                    notify =
+                      rm_result != mtrie_t::values_remain || verbose_unsubs;
+                } else {
+                    bool first_added =
+                      subscriptions.add (data + 1, size - 1, pipe_);
+                    notify = first_added || verbose_subs;
+                }
+
+                //  If the request was a new subscription, or the subscription
+                //  was removed, or verbose mode is enabled, store it so that
+                //  it can be passed to the user on next recv call.
+                if (options.type == ZMQ_XPUB && notify) {
                     pending_data.push_back (blob_t (data, size));
                     if (metadata)
                         metadata->add_ref ();

unittests/unittest_mtrie.cpp

@@ -153,8 +153,9 @@ void test_add_rm_single_entry_match_exact ()
       reinterpret_cast<zmq::generic_mtrie_t<int>::prefix_t> ("foo");
 
     mtrie.add (test_name, getlen (test_name), &pipe);
-    bool res = mtrie.rm (test_name, getlen (test_name), &pipe);
-    TEST_ASSERT_TRUE (res);
+    zmq::generic_mtrie_t<int>::rm_result res =
+      mtrie.rm (test_name, getlen (test_name), &pipe);
+    TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::last_value_removed, res);
 
     int count = 0;
     mtrie.match (test_name, getlen (test_name), mtrie_count, &count);
@@ -166,10 +167,8 @@ void test_rm_nonexistent_0_size_empty ()
     int pipe;
     zmq::generic_mtrie_t<int> mtrie;
 
-    // TODO why does this return true, but test_rm_nonexistent_empty returns false?
-    // or is this not a legal call at all?
-    bool res = mtrie.rm (0, 0, &pipe);
-    TEST_ASSERT_TRUE (res);
+    zmq::generic_mtrie_t<int>::rm_result res = mtrie.rm (0, 0, &pipe);
+    TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::not_found, res);
 }
 
 void test_rm_nonexistent_empty ()
@@ -179,8 +178,9 @@ void test_rm_nonexistent_empty ()
     const zmq::generic_mtrie_t<int>::prefix_t test_name =
       reinterpret_cast<zmq::generic_mtrie_t<int>::prefix_t> ("foo");
 
-    bool res = mtrie.rm (test_name, getlen (test_name), &pipe);
-    TEST_ASSERT_FALSE (res);
+    zmq::generic_mtrie_t<int>::rm_result res =
+      mtrie.rm (test_name, getlen (test_name), &pipe);
+    TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::not_found, res);
 
     int count = 0;
     mtrie.match (test_name, getlen (test_name), mtrie_count, &count);
@@ -198,8 +198,9 @@ void test_add_and_rm_other (const char *add_name, const char *rm_name)
 
     mtrie.add (add_name_data, getlen (add_name_data), &addpipe);
 
-    bool res = mtrie.rm (rm_name_data, getlen (rm_name_data), &rmpipe);
-    TEST_ASSERT_FALSE (res);
+    zmq::generic_mtrie_t<int>::rm_result res =
+      mtrie.rm (rm_name_data, getlen (rm_name_data), &rmpipe);
+    TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::not_found, res);
 
     {
         int count = 0;
@@ -248,7 +249,7 @@ void add_indexed_expect_unique (zmq::generic_mtrie_t<int> &mtrie,
       reinterpret_cast<zmq::generic_mtrie_t<int>::prefix_t> (names[i]);
 
     bool res = mtrie.add (name_data, getlen (name_data), &pipes[i]);
-    TEST_ASSERT_TRUE (res);
+    TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::last_value_removed, res);
 }
 
 void test_rm_nonexistent_between ()
@@ -263,8 +264,9 @@ void test_rm_nonexistent_between ()
     const zmq::generic_mtrie_t<int>::prefix_t name_data =
       reinterpret_cast<zmq::generic_mtrie_t<int>::prefix_t> (names[1]);
 
-    bool res = mtrie.rm (name_data, getlen (name_data), &pipes[1]);
-    TEST_ASSERT_FALSE (res);
+    zmq::generic_mtrie_t<int>::rm_result res =
+      mtrie.rm (name_data, getlen (name_data), &pipes[1]);
+    TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::not_found, res);
 }
 
 template <size_t N>
@@ -323,8 +325,9 @@ template <size_t N> void add_and_rm_entries (const char *(&names)[N])
         const zmq::generic_mtrie_t<int>::prefix_t name_data =
           reinterpret_cast<zmq::generic_mtrie_t<int>::prefix_t> (names[i]);
 
-        bool res = mtrie.rm (name_data, getlen (name_data), &pipes[i]);
-        TEST_ASSERT_TRUE (res);
+        zmq::generic_mtrie_t<int>::rm_result res =
+          mtrie.rm (name_data, getlen (name_data), &pipes[i]);
+        TEST_ASSERT_EQUAL (zmq::generic_mtrie_t<int>::last_value_removed, res);
     }
 }
 
@@ -434,13 +437,9 @@ int main (void)
 
     RUN_TEST (test_rm_nonexistent_0_size_empty);
     RUN_TEST (test_rm_nonexistent_empty);
-#if 0
     RUN_TEST (test_rm_nonexistent_nonempty_samename);
-#endif
     RUN_TEST (test_rm_nonexistent_nonempty_differentname);
-#if 0
     RUN_TEST (test_rm_nonexistent_nonempty_prefix);
-#endif
     RUN_TEST (test_rm_nonexistent_nonempty_prefixed);
     RUN_TEST (test_rm_nonexistent_between);