Refactor Nodes class; WIP: dereferencing the iterator in opetor++

Author: d-kad

src/games/game.h

@@ -514,73 +514,94 @@ class GameRep : public BaseGameRep {
   using Outcomes = ElementCollection<Game, GameOutcomeRep>;
 
   class Nodes {
-  private:
-    using ChildIterator = ElementCollection<GameNode, GameNodeRep>::iterator;
-
     Game m_owner{nullptr};
-    GameNode m_current_node{nullptr};
-    std::stack<std::pair<ChildIterator, ChildIterator>> m_stack{};
-
-    Nodes(Game game) : m_owner(game) {}
 
   public:
-    Nodes() = default;
-
-    Nodes(Game game, GameNode start_node) : m_owner(game), m_current_node(start_node)
-    {
-      if (!start_node) {
-        return;
-      }
-      if (start_node->GetGame() != m_owner) {
-        throw MismatchException();
+    class iterator {
+      friend class Nodes;
+      using ChildIterator = ElementCollection<GameNode, GameNodeRep>::iterator;
+
+      Game m_owner{nullptr};
+      GameNode m_current_node{nullptr};
+      std::stack<ChildIterator> m_stack{};
+
+      iterator(Game game) : m_owner(game) {}
+
+    public:
+      using iterator_category = std::forward_iterator_tag;
+      using value_type = GameNode;
+      using pointer = value_type *;
+
+      iterator() = default;
+
+      iterator(Game game, GameNode start_node) : m_owner(game), m_current_node(start_node)
+      {
+        if (!start_node) {
+          return;
+        }
+        if (start_node->GetGame() != m_owner) {
+          throw MismatchException();
+        }
       }
-      if (start_node != m_owner->GetRoot()) {
-        throw std::invalid_argument("Node iteration can only be initiated from the game's root");
-      }
-    }
 
-    GameNode operator*() const
-    {
-      if (!m_current_node) {
-        throw std::runtime_error("Cannot dereference an end iterator");
+      value_type operator*() const
+      {
+        if (!m_current_node) {
+          throw std::runtime_error("Cannot dereference an end iterator");
+        }
+        return m_current_node;
       }
-      return m_current_node;
-    }
 
-    Nodes &operator++()
-    {
-      if (!m_current_node) {
-        throw std::out_of_range("Cannot increment an end iterator");
+      iterator &operator++()
+      {
+        if (!m_current_node) {
+          throw std::out_of_range("Cannot increment an end iterator");
+        }
+
+        auto children = m_current_node->GetChildren();
+        if (children.size() > 0) {
+          m_stack.emplace(children.begin());
+        }
+
+        while (!m_stack.empty()) {
+          try {
+            *m_stack.top();
+            break;
+          }
+          catch (std::out_of_range) {
+            m_stack.pop();
+          }
+        }
+
+        if (m_stack.empty()) {
+          m_current_node = nullptr;
+        }
+        else {
+          auto &top_it = m_stack.top();
+          m_current_node = *top_it;
+          ++top_it;
+        }
+        return *this;
       }
 
-      auto children = m_current_node->GetChildren();
-      if (children.size() > 0) {
-        m_stack.emplace(children.begin(), children.end());
+      bool operator==(const iterator &other) const
+      {
+        return m_owner == other.m_owner && m_current_node == other.m_current_node;
       }
+      bool operator!=(const iterator &other) const { return !(*this == other); }
+    };
 
-      while (!m_stack.empty() && m_stack.top().first == m_stack.top().second) {
-        m_stack.pop();
-      }
-
-      if (m_stack.empty()) {
-        m_current_node = nullptr;
-      }
-      else {
-        auto &top_pair = m_stack.top();
-        m_current_node = *(top_pair.first);
-        ++(top_pair.first);
-      }
-      return *this;
-    }
+    /// Default constructor to support declaration by other modules (e.g. Cython)
+    Nodes() = default;
 
-    bool operator==(const Nodes &other) const
-    {
-      return m_owner == other.m_owner && m_current_node == other.m_current_node;
-    }
+    /// Constructor for the Nodes range.
+    explicit Nodes(Game p_owner) : m_owner(p_owner) {}
 
-    bool operator!=(const Nodes &other) const { return !(*this == other); }
+    /// Returns an iterator to the first node (the root).
+    iterator begin() const { return m_owner ? iterator(m_owner, m_owner->GetRoot()) : iterator(); }
 
-    friend class GameRep;
+    /// Returns an iterator to the past-the-end position.
+    iterator end() const { return m_owner ? iterator(m_owner) : iterator(); }
   };
 
   /// @name Lifecycle
@@ -795,6 +816,8 @@ class GameRep : public BaseGameRep {
   //@{
   /// Returns the root node of the game
   virtual GameNode GetRoot() const = 0;
+  /// Returns the nodes of the game in the DFT order
+  Nodes GetNodes() const { return Nodes(this); }
   /// Returns the number of nodes in the game
   virtual size_t NumNodes() const = 0;
   /// Returns the number of non-terminal nodes in the game
@@ -807,10 +830,6 @@ class GameRep : public BaseGameRep {
   virtual Game SetChanceProbs(const GameInfoset &, const Array<Number> &) = 0;
   //@}
 
-  /// Node iterators
-  Nodes begin() const { return {this, GetRoot()}; }
-  Nodes end() const { return {this}; }
-
   /// Build any computed values anew
   virtual void BuildComputedValues() const {}
 };

src/pygambit/gambit.pxd

@@ -242,10 +242,13 @@ cdef extern from "games/game.h":
             iterator end() except +
 
         cppclass Nodes:
-            bint operator ==(Nodes)
-            bint operator !=(Nodes)
-            c_GameNode operator *()
-            Nodes operator++()
+            cppclass iterator:
+                bint operator ==(iterator)
+                bint operator !=(iterator)
+                c_GameNode operator *()
+                iterator operator++()
+            iterator begin() except +
+            iterator end() except +
 
         int IsTree() except +
 
@@ -270,6 +273,7 @@ cdef extern from "games/game.h":
         int NumNodes() except +
         int NumNonterminalNodes() except +
         c_GameNode GetRoot() except +
+        Nodes GetNodes() except +
 
         c_GameStrategy GetStrategy(int) except +IndexError
         c_GameStrategy NewStrategy(c_GamePlayer, string) except +
@@ -308,8 +312,6 @@ cdef extern from "games/game.h":
         void DeleteAction(c_GameAction) except +ValueError
         void SetOutcome(c_GameNode, c_GameOutcome) except +
         c_Game SetChanceProbs(c_GameInfoset, Array[c_Number]) except +
-        Nodes begin() except +
-        Nodes end() except +
 
         c_PureStrategyProfile NewPureStrategyProfile()  # except + doesn't compile
         c_MixedStrategyProfile[T] NewMixedStrategyProfile[T](T)  # except + doesn't compile

src/pygambit/game.pxi

@@ -186,20 +186,12 @@ class GameNodes:
         return self.game.deref().NumNodes()
 
     def __iter__(self) -> typing.Iterator[Node]:
-        """
-        A generator that efficiently iterates over the game nodes using
-        the underlying C++ iterator, without using cdef for local variables.
-        """
+        """Iterate over the game nodes in the depth-first traversal order."""
         if not self.game.deref().IsTree():
             return
 
-        it = self.game.deref().begin()
-        end_it = self.game.deref().end()
-
-        while it != end_it:
-            yield Node.wrap(dereference(it))
-
-            preincrement(it)
+        for node in self.game.deref().GetNodes():
+            yield Node.wrap(node)
 
 
 @cython.cclass