From 5fb0e97ab71a49170c04877debc6cf48db9d3110 Mon Sep 17 00:00:00 2001
From: Simon Giesecke <simon.giesecke@btc-ag.com>
Date: Wed, 21 Feb 2018 11:31:00 +0100
Subject: [PATCH] Problem: semantic issues

Solution: added some TODO comments, improved existing documentation
---
 src/generic_mtrie.hpp      | 13 +++++++++----
 src/generic_mtrie_impl.hpp |  8 ++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/generic_mtrie.hpp b/src/generic_mtrie.hpp
index d40208e7..b0c36041 100644
--- a/src/generic_mtrie.hpp
+++ b/src/generic_mtrie.hpp
@@ -48,21 +48,26 @@ template <typename T> class generic_mtrie_t
     ~generic_mtrie_t ();
 
     //  Add key to the trie. Returns true if it's a new subscription
-    //  rather than a duplicate.
+    //  rather than a duplicate (i.e. an entry with the same prefix
+    //  but a different pipe already exists).
+    //  TODO what if this is called with the same prefix AND pipe?
+    //  Is this legal? It is not checked anywhere.
     bool add (prefix_t prefix_, size_t size_, value_t *pipe_);
 
     //  Remove all subscriptions for a specific peer from the trie.
     //  The call_on_uniq_ flag controls if the callback is invoked
-    //  when there are no subscriptions left on some topics or on
-    //  every removal.
+    //  when there are no subscriptions left on a topic only (true)
+    //  or on every removal (false).
     void
     rm (value_t *pipe_,
         void (*func_) (const unsigned char *data_, size_t size_, void *arg_),
         void *arg_,
         bool call_on_uniq_);
 
-    //  Remove specific subscription from the trie. Return true is it was
+    //  Remove specific subscription 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/pipe
+    //  pair was not found must be specified
     bool rm (prefix_t prefix_, size_t size_, value_t *pipe_);
 
     //  Signal all the matching pipes.
diff --git a/src/generic_mtrie_impl.hpp b/src/generic_mtrie_impl.hpp
index f17df75c..c28f837c 100644
--- a/src/generic_mtrie_impl.hpp
+++ b/src/generic_mtrie_impl.hpp
@@ -305,8 +305,16 @@ bool zmq::generic_mtrie_t<T>::rm_helper (prefix_t prefix_,
                                          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.
             zmq_assert (erased == 1);
             if (pipes->empty ()) {
                 LIBZMQ_DELETE (pipes);