Perform a bunch of minor cleanup.

Add some documentation of the trie class.
Fix a bug where post-order wasn't behaving right.
Make iterator memvars private, give better names.

trie.hpp

@@ -23,6 +23,24 @@ namespace detail {
 template <typename Trie, typename Iter> class trie_iterator;
 template <typename Trie, typename Iter> class trie_reverse_iterator;
 
+/*
+ * \brief A trie is a type of associative container where each element is
+ * locatable by a string of keys, including a zero-length string. Sometimes
+ * known as a prefix-tree.
+ * \tparam K The key type. The container is indexable by both single keys, and
+ * iterable collections of keys e.g. std::initializer_list<K>.
+ * \tparam V The value type of stored data.
+ * \tparam Compare The comparator function, used by the backing std::map object
+ * to order this container.
+ *
+ * In principle, a trie<K, V> is equal to a std::map<std::vector<K>, V> with the
+ * additional features of being able to locate all elements whose keys start
+ * with the key fragment { K1, K2, ..., Kn }.
+ *
+ * Because the conceptual structure of a trie is a tree, both pre-order and
+ * port-order iteration are supported. In-order traversal is not supported,
+ * because it doesn't translate well with non-binary trees.
+ */
 template <typename K, typename V, typename Compare> class trie {
 private:
   using self_t = trie<K, V, Compare>;
@@ -47,9 +65,9 @@ public:
 
   using iterator = trie_iterator<self_t, local_iterator>;
   using const_iterator = trie_iterator<self_t const, local_const_iterator>;
-  using post_iterator = trie_iterator<self_t, local_reverse_iterator>;
+  using post_iterator = trie_reverse_iterator<self_t, local_iterator>;
   using const_post_iterator =
-      trie_iterator<self_t const, local_const_reverse_iterator>;
+      trie_reverse_iterator<self_t const, local_const_iterator>;
   using reverse_iterator =
       trie_reverse_iterator<self_t, local_reverse_iterator>;
   using const_reverse_iterator =
@@ -76,10 +94,10 @@ public:
 
   template <typename KS, typename = is_collection_t<KS>>
   self_t & operator[](KS const & keys) {
-    return *emplace(keys).first.stk.top();
+    return *emplace(keys).first.parent_trie_.top();
   }
   self_t & operator[](key_type const & key) {
-    return *emplace(key).first.stk.top();
+    return *emplace(key).first.parent_trie_.top();
   }
   self_t & operator[](std::initializer_list<key_type> keys) {
     return operator[]<>(keys);
@@ -189,8 +207,8 @@ private:
     const_iterator it1 = lhs.begin(), it2 = rhs.begin();
     const_iterator const end1 = lhs.end(), end2 = lhs.end();
     for (; it1 != end1 && it2 != end2; ++it1, ++it2) {
-      if (!it1.keys.empty() && !it2.keys.empty() &&
-          it1.keys.back() != it2.keys.back()) {
+      if (!it1.keys_.empty() && !it2.keys_.empty() &&
+          it1.keys_.back() != it2.keys_.back()) {
         return false;
       } else if (it1.root().value_ != it2.root().value_) {
         return false;
@@ -199,6 +217,10 @@ private:
     return it1 == end1 && it2 == end2;
   }
 
+  friend bool operator!=(trie const & lhs, trie const & rhs) {
+    return !(lhs == rhs);
+  }
+
   friend void swap(trie & lhs, trie & rhs) {
     using std::swap;
     swap(lhs.value_, rhs.value_);
@@ -209,4 +231,4 @@ private:
   backing_t impl_{};
 };
 
-#include "trie_impl.hpp"
+#include "trie.tpp"

trie_impl.hpp

@@ -18,11 +18,11 @@ template <typename K, typename V, typename C>
 trie<K, V, C>::trie(trie const & other) : value_(other.value_) {
   impl_iterator current{this};
   for (const_iterator it = ++other.begin(), end = {}; it != end; ++it) {
-    while (current.keys.size() >= it.keys.size()) {
+    while (current.keys_.size() >= it.keys_.size()) {
       current.pop();
     }
-    auto tmp = current.root().insert(it.keys.back(), *it).first;
-    current.push(tmp.iters.top());
+    auto tmp = current.root().insert(it.keys_.back(), *it).first;
+    current.push(tmp.current());
   }
 }
 
@@ -81,9 +81,9 @@ auto trie<K, V, C>::emplace_impl(KS && keys, Args &&... args)
 // Stack: O(1)
 // Operations: O(n)
 template <typename K, typename V, typename C> void trie<K, V, C>::clear() {
-  for (reverse_iterator it = rbegin(), end = rend();
-       it != end && !it.iters.empty(); ++it) {
-    auto ptr = std::move(it.iters.top()->second);
+  for (reverse_iterator it = rbegin(), end = rend(); it != end && !it.empty();
+       ++it) {
+    auto ptr = std::move(it.current()->second);
     ptr->impl_.clear();
   }
   value_ = mapped_type{};
@@ -94,8 +94,8 @@ template <typename K, typename V, typename C>
 void trie<K, V, C>::drop(iterator it) {
   if (it == end()) return;
   it.root().clear();
-  if (!it.iters.empty()) {
-    auto to_erase = it.iters.top().current();
+  if (!it.empty()) {
+    auto to_erase = it.current().current();
     it.pop();
     it.root().impl_.erase(to_erase);
   }

trie.xcodeproj/project.pbxproj

@@ -9,7 +9,7 @@
 /* Begin PBXBuildFile section */
 		CD0C596320C2DC3200454F82 /* GoogleMock.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = CD0C594D20C2DBE700454F82 /* GoogleMock.framework */; };
 		CD0C596420C2DC3800454F82 /* trie_test.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CD0C595420C2DBFB00454F82 /* trie_test.cpp */; };
-		CD0C596D20C2E9DE00454F82 /* trie_impl.hpp in Headers */ = {isa = PBXBuildFile; fileRef = CD46DF481EF47C520092D121 /* trie_impl.hpp */; };
+		CD0C596D20C2E9DE00454F82 /* trie.tpp in Headers */ = {isa = PBXBuildFile; fileRef = CD46DF481EF47C520092D121 /* trie.tpp */; };
 		CD0C596E20C2E9E300454F82 /* trie.hpp in Headers */ = {isa = PBXBuildFile; fileRef = CD46DF451EF3FDCE0092D121 /* trie.hpp */; };
 		CD0C596F20C2E9E300454F82 /* trie_iterator.hpp in Headers */ = {isa = PBXBuildFile; fileRef = CD46DF4A1EF497E30092D121 /* trie_iterator.hpp */; };
 		CD0C597420C2EA7300454F82 /* trie_dummy.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CD0C597320C2EA7300454F82 /* trie_dummy.cpp */; };
@@ -69,7 +69,7 @@
 		CD0C596920C2E9CE00454F82 /* libtrie.dylib */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.dylib"; includeInIndex = 0; path = libtrie.dylib; sourceTree = BUILT_PRODUCTS_DIR; };
 		CD0C597320C2EA7300454F82 /* trie_dummy.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = trie_dummy.cpp; sourceTree = "<group>"; };
 		CD46DF451EF3FDCE0092D121 /* trie.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = trie.hpp; sourceTree = "<group>"; };
-		CD46DF481EF47C520092D121 /* trie_impl.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = trie_impl.hpp; sourceTree = "<group>"; };
+		CD46DF481EF47C520092D121 /* trie.tpp */ = {isa = PBXFileReference; explicitFileType = sourcecode.cpp.h; path = trie.tpp; sourceTree = "<group>"; };
 		CD46DF4A1EF497E30092D121 /* trie_iterator.hpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.h; path = trie_iterator.hpp; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
@@ -137,7 +137,7 @@
 			children = (
 				CD46DF451EF3FDCE0092D121 /* trie.hpp */,
 				CD46DF4A1EF497E30092D121 /* trie_iterator.hpp */,
-				CD46DF481EF47C520092D121 /* trie_impl.hpp */,
+				CD46DF481EF47C520092D121 /* trie.tpp */,
 				CD0C597320C2EA7300454F82 /* trie_dummy.cpp */,
 			);
 			name = src;
@@ -151,7 +151,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				CD0C596E20C2E9E300454F82 /* trie.hpp in Headers */,
-				CD0C596D20C2E9DE00454F82 /* trie_impl.hpp in Headers */,
+				CD0C596D20C2E9DE00454F82 /* trie.tpp in Headers */,
 				CD0C596F20C2E9E300454F82 /* trie_iterator.hpp in Headers */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;

trie_iterator.hpp

@@ -9,9 +9,10 @@
 
 #include "trie.hpp"
 
-#include "iterator/end_aware_iterator.hpp"
-#include <list>
 #include <stack>
+#include <vector>
+
+#include "iterator/end_aware_iterator.hpp"
 
 namespace detail {
   template <typename Iter> struct trie_iterator_next {
@@ -30,13 +31,11 @@ namespace detail {
   };
 
   template <typename Trie, typename Iter> class trie_iterator_base {
-  protected:
-    using helper = trie_iterator_next<Iter>;
-    using impl_t = typename helper::iter_t;
-    std::stack<Trie *> stk;
-    std::list<typename Trie::key_type> keys;
-    std::stack<impl_t> iters;
-    bool done{false};
+  private:
+    std::stack<Trie *> parent_trie_;
+    std::vector<typename Trie::key_type> keys_;
+    std::stack<iterator::end_aware_iterator<Iter>> iterators_;
+    bool done_{false};
 
   public:
     using reference = decltype(std::declval<Trie>().value());
@@ -46,16 +45,18 @@ namespace detail {
     using iterator_category = std::forward_iterator_tag;
 
   public:
-    trie_iterator_base() : done{true} {}
-    trie_iterator_base(Trie * tr) { stk.push(tr); }
+    trie_iterator_base() : done_{true} {}
+    trie_iterator_base(Trie * tr) { parent_trie_.push(tr); }
     auto operator*() const -> reference { return root(); }
     auto operator-> () const -> pointer { return std::addressof(operator*()); }
+
     Trie & root() const {
-      if (stk.empty()) {
+      if (parent_trie_.empty()) {
         throw std::runtime_error("Dereferencing invalid iterator");
       }
-      return *stk.top();
+      return *parent_trie_.top();
     }
+
     trie_iterator_base parent() const {
       trie_iterator_base tmp{*this};
       tmp.pop();
@@ -63,42 +64,58 @@ namespace detail {
     }
 
   protected:
-    void push(impl_t it) {
+    iterator::end_aware_iterator<Iter> & current() { return iterators_.top(); }
+    bool empty() const { return iterators_.empty(); }
+    void done(bool new_done) { done_ = new_done; }
+    bool done() const { return done_; }
+
+    void push(iterator::end_aware_iterator<Iter> it) {
       if (it.done()) {
-        done = true;
+        done_ = true;
         return;
       }
-      iters.push(it);
-      keys.push_back(it->first);
-      stk.push(it->second.get());
+      iterators_.push(it);
+      keys_.push_back(it->first);
+      parent_trie_.push(it->second.get());
     }
+
     void pop() {
-      stk.pop();
-      iters.pop();
-      keys.pop_back();
+      parent_trie_.pop();
+      iterators_.pop();
+      keys_.pop_back();
     }
+
     bool poping_empty() {
-      if (!iters.top().done()) { return false; }
+      if (!current().done()) { return false; }
       pop();
-      return !iters.empty();
+      return !empty();
     }
+
     void assign() {
-      if (iters.empty()) { return; }
-      stk.top() = iters.top()->second.get();
-      keys.back() = iters.top()->first;
+      if (empty()) { return; }
+      parent_trie_.top() = current()->second.get();
+      keys_.back() = current()->first;
     }
+
     bool can_recurse() { return !next().done(); }
+
     void recurse() { push(next()); }
-    impl_t next() { return helper()(root()); }
+
+    iterator::end_aware_iterator<Iter> next() {
+      return trie_iterator_next<Iter>()(root());
+    }
+
     friend bool operator!=(trie_iterator_base const & lhs,
                            trie_iterator_base const & rhs) {
       return !(lhs == rhs);
     }
+
     friend bool operator==(trie_iterator_base const & lhs,
                            trie_iterator_base const & rhs) {
-      return (lhs.done && rhs.done) ||
-             (lhs.keys == rhs.keys && lhs.done == rhs.done && *lhs == *rhs);
+      return (lhs.done_ && rhs.done_) ||
+             (lhs.keys_ == rhs.keys_ && lhs.done_ == rhs.done_ && *lhs == *rhs);
     }
+
     friend Trie;
     template <typename Self, typename _Trie, typename KS>
     friend Self find_impl(_Trie * tr, KS const & keys);
@@ -114,9 +131,10 @@ public:
   trie_iterator() : super() {}
   trie_iterator(Trie * tr) : super(tr) {}
   trie_iterator(super && take) : super(std::move(take)) {}
+
   trie_iterator & operator++() {
-    if (super::done) return *this;
-    if (super::iters.empty() || super::can_recurse()) {
+    if (super::done()) return *this;
+    if (super::empty() || super::can_recurse()) {
       super::recurse();
     } else {
       advance();
@@ -126,14 +144,13 @@ public:
 
 private:
   void advance() {
-    ++super::iters.top();
+    ++super::current();
     while (super::poping_empty()) {
-      ++super::iters.top();
+      ++super::current();
     }
     super::assign();
-    super::done = super::iters.empty();
+    super::done(super::empty());
   }
-  friend Trie;
 };
 
 template <typename Trie, typename Iter>
@@ -146,9 +163,10 @@ public:
   trie_reverse_iterator(Trie * tr) : super(tr) {
     if (super::can_recurse()) recurse();
   }
+
   trie_reverse_iterator & operator++() {
-    if (super::done || super::iters.empty()) {
-      super::done = true;
+    if (super::done() || super::empty()) {
+      super::done(true);
     } else {
       advance();
     }
@@ -161,9 +179,10 @@ private:
       super::recurse();
     } while (super::can_recurse());
   }
+
   void advance() {
-    ++super::iters.top();
-    if (super::iters.top().done()) {
+    ++super::current();
+    if (super::current().done()) {
       while (super::poping_empty())
         ;
       super::assign();
@@ -172,5 +191,4 @@ private:
       if (super::can_recurse()) recurse();
     }
   }
-  friend Trie;
 };

trie_test.cpp

@@ -55,15 +55,24 @@ TEST_F(TrieTest, MoveAssignmentOverwritesAnfRelocates) {
   value[2] = 1;
   value = std::move(data);
   EXPECT_THAT(value.find(2), value.end());
-  EXPECT_THAT(value, ::testing::Not(data));
+  EXPECT_THAT(value, ::testing::Ne(data));
 }
 
+/**
+ * This is important because a trie is implemented as a
+ * map<key_type, self_type*>. Therefore, a naive copy-ctor would result in the
+ * copy having the same data after the first level (root value and inserts with
+ * one key wouldn't backflow, but everything else would).
+ */
 TEST_F(TrieTest, CopyConstructorIsDeep) {
   trie<int, int> copy = data;
   copy[{0, 1}] += 1;
-  EXPECT_THAT(copy, ::testing::Not(::testing::Eq(data)));
+  EXPECT_THAT(copy, ::testing::Ne(data));
 }
 
+/**
+ * Ensure that copy-assign doesn't simply add-all.
+ */
 TEST_F(TrieTest, CopyAssignmentOverwritesData) {
   trie<int, int> value;
   value[2] = 1;
@@ -76,7 +85,7 @@ TEST_F(TrieTest, EqualityIsPathSensitive) {
   t1[1] = 1;
   t2[2] = 1;
   EXPECT_THAT(flatten(t1.cbegin()), flatten(t2.cbegin()));
-  EXPECT_THAT(t1, ::testing::Not(::testing::Eq(t2)));
+  EXPECT_THAT(t1, ::testing::Ne(t2));
 }
 
 TEST_F(TrieTest, NormalIterationIsPreOrdered) {
@@ -86,7 +95,7 @@ TEST_F(TrieTest, NormalIterationIsPreOrdered) {
 
 TEST_F(TrieTest, PostIteratorUsesPostOrder) {
   EXPECT_THAT(flatten(trie<int, int>::const_post_iterator(&data)),
-              std::vector<int>({1, 2, 8, 9, 5, 0, 7, 4, 6, 3}));
+              std::vector<int>({3, 6, 4, 7, 0, 5, 9, 8, 2, 1}));
 }
 
 TEST_F(TrieTest, ReverseIterationIsBackwardsPreOrdered) {
@@ -114,7 +123,9 @@ TEST_F(TrieTest, InsertingWithPathWillCreateParents) {
       example.emplace({1, 1, 1}, 2);
   EXPECT_TRUE(rval.second);
   EXPECT_THAT(*rval.first, 2);
-  EXPECT_THAT(example[1][1][1].value(), 2);
+  EXPECT_THAT(example.find({1}), testing::Ne(example.end()));
+  EXPECT_THAT(example.find({1, 1}), testing::Ne(example.end()));
+  EXPECT_THAT(example.find({1, 1, 1}), rval.first);
 }
 
 TEST_F(TrieTest, InsertingInAlreadyExistantPathNonDestructive) {
@@ -146,7 +157,7 @@ TEST_F(TrieTest, DereferenceWithMissingPathThrows) {
 }
 
 TEST_F(TrieTest, EraseDropsWholeBranch) {
-  EXPECT_THAT(data.find({0, 1}), ::testing::Not(data.end()));
+  EXPECT_THAT(data.find({0, 1}), ::testing::Ne(data.end()));
   data.erase(0);
   EXPECT_THAT(data.find({0, 1}), data.end());
   EXPECT_THAT(data.find(0), data.end());