diff --git a/src/generic_mtrie.hpp b/src/generic_mtrie.hpp index c35abc62..623eabf5 100644 --- a/src/generic_mtrie.hpp +++ b/src/generic_mtrie.hpp @@ -44,12 +44,18 @@ template 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 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 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 pipes_t; diff --git a/src/generic_mtrie_impl.hpp b/src/generic_mtrie_impl.hpp index 1b293b45..0e942d1f 100644 --- a/src/generic_mtrie_impl.hpp +++ b/src/generic_mtrie_impl.hpp @@ -294,47 +294,39 @@ void zmq::generic_mtrie_t::rm_helper(value_t *pipe_, } template -bool zmq::generic_mtrie_t::rm (prefix_t prefix_, - size_t size_, - value_t *pipe_) +typename zmq::generic_mtrie_t::rm_result +zmq::generic_mtrie_t::rm (prefix_t prefix_, size_t size_, value_t *pipe_) { return rm_helper (prefix_, size_, pipe_); } template -bool zmq::generic_mtrie_t::rm_helper (prefix_t prefix_, - size_t size_, - value_t *pipe_) +typename zmq::generic_mtrie_t::rm_result zmq::generic_mtrie_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); diff --git a/src/xpub.cpp b/src/xpub.cpp index 74cd9167..03baa2da 100644 --- a/src/xpub.cpp +++ b/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_); + 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 (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))) { + // 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 (); diff --git a/unittests/unittest_mtrie.cpp b/unittests/unittest_mtrie.cpp index 8fc4c22b..8302182c 100644 --- a/unittests/unittest_mtrie.cpp +++ b/unittests/unittest_mtrie.cpp @@ -153,8 +153,9 @@ void test_add_rm_single_entry_match_exact () reinterpret_cast::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::rm_result res = + mtrie.rm (test_name, getlen (test_name), &pipe); + TEST_ASSERT_EQUAL (zmq::generic_mtrie_t::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 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::rm_result res = mtrie.rm (0, 0, &pipe); + TEST_ASSERT_EQUAL (zmq::generic_mtrie_t::not_found, res); } void test_rm_nonexistent_empty () @@ -179,8 +178,9 @@ void test_rm_nonexistent_empty () const zmq::generic_mtrie_t::prefix_t test_name = reinterpret_cast::prefix_t> ("foo"); - bool res = mtrie.rm (test_name, getlen (test_name), &pipe); - TEST_ASSERT_FALSE (res); + zmq::generic_mtrie_t::rm_result res = + mtrie.rm (test_name, getlen (test_name), &pipe); + TEST_ASSERT_EQUAL (zmq::generic_mtrie_t::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::rm_result res = + mtrie.rm (rm_name_data, getlen (rm_name_data), &rmpipe); + TEST_ASSERT_EQUAL (zmq::generic_mtrie_t::not_found, res); { int count = 0; @@ -248,7 +249,7 @@ void add_indexed_expect_unique (zmq::generic_mtrie_t &mtrie, reinterpret_cast::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::last_value_removed, res); } void test_rm_nonexistent_between () @@ -263,8 +264,9 @@ void test_rm_nonexistent_between () const zmq::generic_mtrie_t::prefix_t name_data = reinterpret_cast::prefix_t> (names[1]); - bool res = mtrie.rm (name_data, getlen (name_data), &pipes[1]); - TEST_ASSERT_FALSE (res); + zmq::generic_mtrie_t::rm_result res = + mtrie.rm (name_data, getlen (name_data), &pipes[1]); + TEST_ASSERT_EQUAL (zmq::generic_mtrie_t::not_found, res); } template @@ -323,8 +325,9 @@ template void add_and_rm_entries (const char *(&names)[N]) const zmq::generic_mtrie_t::prefix_t name_data = reinterpret_cast::prefix_t> (names[i]); - bool res = mtrie.rm (name_data, getlen (name_data), &pipes[i]); - TEST_ASSERT_TRUE (res); + zmq::generic_mtrie_t::rm_result res = + mtrie.rm (name_data, getlen (name_data), &pipes[i]); + TEST_ASSERT_EQUAL (zmq::generic_mtrie_t::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);