Separate validation from memory management for actions.

Author: tturocy

src/games/game.h

@@ -23,6 +23,7 @@
 #ifndef LIBGAMBIT_GAME_H
 #define LIBGAMBIT_GAME_H
 
+#include <iostream>
 #include <list>
 #include <set>
 
@@ -38,7 +39,7 @@ class GameOutcomeRep;
 using GameOutcome = GameObjectPtr<GameOutcomeRep>;
 
 class GameActionRep;
-using GameAction = GameObjectPtr<GameActionRep>;
+using GameAction = GameObjectSharedPtr<GameActionRep>;
 
 class GameInfosetRep;
 using GameInfoset = GameObjectPtr<GameInfosetRep>;
@@ -148,7 +149,7 @@ class GameOutcomeRep : public GameObject {
 };
 
 /// An action at an information set in an extensive game
-class GameActionRep : public GameObject {
+class GameActionRep : public GameValidObject, public std::enable_shared_from_this<GameActionRep> {
   friend class GameTreeRep;
   friend class GameInfosetRep;
   template <class T> friend class MixedBehaviorProfile;
@@ -157,13 +158,13 @@ class GameActionRep : public GameObject {
   std::string m_label;
   GameInfosetRep *m_infoset;
 
+public:
   GameActionRep(int p_number, const std::string &p_label, GameInfosetRep *p_infoset)
     : m_number(p_number), m_label(p_label), m_infoset(p_infoset)
   {
   }
-  ~GameActionRep() override = default;
+  ~GameActionRep() = default;
 
-public:
   int GetNumber() const { return m_number; }
   GameInfoset GetInfoset() const;
 
@@ -184,7 +185,7 @@ class GameInfosetRep : public GameObject {
   int m_number;
   std::string m_label;
   GamePlayerRep *m_player;
-  Array<GameActionRep *> m_actions;
+  Array<std::shared_ptr<GameActionRep>> m_actions;
   std::vector<GameNodeRep *> m_members;
   int flag{0}, whichbranch{0};
   Array<Number> m_probs;
@@ -195,7 +196,7 @@ class GameInfosetRep : public GameObject {
   void RenumberActions()
   {
     std::for_each(m_actions.begin(), m_actions.end(),
-                  [act = 1](GameActionRep *a) mutable { a->m_number = act++; });
+                  [act = 1](std::shared_ptr<GameActionRep> a) mutable { a->m_number = act++; });
   }
 
 public:

src/games/gameobject.h

@@ -23,6 +23,8 @@
 #ifndef GAMBIT_GAMES_GAMEOBJECT_H
 #define GAMBIT_GAMES_GAMEOBJECT_H
 
+#include <memory>
+
 namespace Gambit {
 
 /// This is a base class for all game-related objects.  Primary among
@@ -84,6 +86,18 @@ class GameObject {
   //@}
 };
 
+class GameValidObject {
+protected:
+  bool m_valid{true};
+
+public:
+  GameValidObject() = default;
+  ~GameValidObject() = default;
+
+  bool IsValid() const { return m_valid; }
+  void Invalidate() { m_valid = false; }
+};
+
 class BaseGameRep {
 protected:
   int m_refCount{0};
@@ -207,6 +221,43 @@ template <class T> class GameObjectPtr {
   bool operator!() const { return !rep; }
 };
 
+template <class T> class GameObjectSharedPtr {
+private:
+  std::shared_ptr<T> m_rep;
+
+public:
+  GameObjectSharedPtr() = default;
+  GameObjectSharedPtr(std::nullptr_t r) : m_rep(r) {}
+  GameObjectSharedPtr(std::shared_ptr<T> r) : m_rep(r) {}
+  GameObjectSharedPtr(const GameObjectSharedPtr<T> &) = default;
+  ~GameObjectSharedPtr() = default;
+
+  GameObjectSharedPtr<T> &operator=(const GameObjectSharedPtr<T> &) = default;
+
+  std::shared_ptr<T> operator->() const
+  {
+    if (!m_rep) {
+      throw NullException();
+    }
+    if (!m_rep->IsValid()) {
+      throw InvalidObjectException();
+    }
+    return m_rep;
+  }
+
+  bool operator==(const GameObjectSharedPtr<T> &r) const { return (m_rep == r.m_rep); }
+  bool operator==(const std::shared_ptr<T> r) const { return (m_rep == r); }
+  bool operator==(const std::shared_ptr<const T> r) const { return (m_rep == r); }
+  bool operator==(const std::nullptr_t) const { return !bool(m_rep); }
+  bool operator!=(const GameObjectSharedPtr<T> &r) const { return (m_rep != r.m_rep); }
+  bool operator!=(const std::shared_ptr<T> r) const { return (m_rep != r); }
+  bool operator!=(const std::shared_ptr<const T> r) const { return (m_rep != r); }
+  bool operator<(const GameObjectSharedPtr<T> &r) const { return (m_rep < r.m_rep); }
+
+  operator bool() const noexcept { return bool(m_rep); }
+  operator std::shared_ptr<T>() const { return m_rep; }
+};
+
 } // end namespace Gambit
 
 #endif // GAMBIT_GAMES_GAMEOBJECT_H

src/games/gametree.cc

@@ -102,7 +102,7 @@ bool GameActionRep::Precedes(const GameNode &n) const
   GameNode node = n;
 
   while (node != node->GetGame()->GetRoot()) {
-    if (node->GetPriorAction() == GameAction(const_cast<GameActionRep *>(this))) {
+    if (node->GetPriorAction() == shared_from_this()) {
       return true;
     }
     else {
@@ -114,7 +114,7 @@ bool GameActionRep::Precedes(const GameNode &n) const
 
 void GameTreeRep::DeleteAction(GameAction p_action)
 {
-  GameActionRep *action = p_action;
+  const std::shared_ptr<GameActionRep> action = p_action;
   auto *infoset = action->m_infoset;
   if (infoset->m_game != this) {
     throw MismatchException();
@@ -156,8 +156,9 @@ GameInfosetRep::GameInfosetRep(GameRep *p_efg, int p_number, GamePlayerRep *p_pl
                                int p_actions)
   : m_game(p_efg), m_number(p_number), m_player(p_player), m_actions(p_actions)
 {
-  std::generate(m_actions.begin(), m_actions.end(),
-                [this, i = 1]() mutable { return new GameActionRep(i++, "", this); });
+  std::generate(m_actions.begin(), m_actions.end(), [this, i = 1]() mutable {
+    return std::make_shared<GameActionRep>(i++, "", this);
+  });
   if (p_player->IsChance()) {
     m_probs = Array<Number>(m_actions.size());
     std::fill(m_probs.begin(), m_probs.end(), Rational(1, m_actions.size()));
@@ -167,7 +168,8 @@ GameInfosetRep::GameInfosetRep(GameRep *p_efg, int p_number, GamePlayerRep *p_pl
 
 GameInfosetRep::~GameInfosetRep()
 {
-  std::for_each(m_actions.begin(), m_actions.end(), [](GameActionRep *a) { a->Invalidate(); });
+  std::for_each(m_actions.begin(), m_actions.end(),
+                [](std::shared_ptr<GameActionRep> a) { a->Invalidate(); });
 }
 
 Game GameInfosetRep::GetGame() const { return m_game; }
@@ -226,11 +228,11 @@ GameAction GameTreeRep::InsertAction(GameInfoset p_infoset, GameAction p_action
   IncrementVersion();
   int where = p_infoset->m_actions.size() + 1;
   if (p_action) {
-    for (where = 1; p_infoset->m_actions[where] != p_action; where++)
+    for (where = 1; p_action != p_infoset->m_actions[where]; where++)
       ;
   }
 
-  auto *action = new GameActionRep(where, "", p_infoset);
+  auto action = std::make_shared<GameActionRep>(where, "", p_infoset);
   p_infoset->m_actions.insert(std::next(p_infoset->m_actions.cbegin(), where - 1), action);
   if (p_infoset->m_player->IsChance()) {
     p_infoset->m_probs.insert(std::next(p_infoset->m_probs.cbegin(), where - 1), Number());
@@ -633,7 +635,7 @@ GameInfoset GameTreeRep::AppendMove(GameNode p_node, GameInfoset p_infoset)
   node->m_infoset = p_infoset;
   node->m_infoset->m_members.push_back(node);
   std::for_each(node->m_infoset->m_actions.begin(), node->m_infoset->m_actions.end(),
-                [this, node](const GameActionRep *) {
+                [this, node](std::shared_ptr<GameActionRep>) {
                   node->m_children.push_back(new GameNodeRep(this, node));
                   m_numNodes++;
                 });
@@ -680,7 +682,8 @@ GameInfoset GameTreeRep::InsertMove(GameNode p_node, GameInfoset p_infoset)
   node->m_parent = newNode;
   newNode->m_children.push_back(node);
   std::for_each(std::next(newNode->m_infoset->m_actions.begin()),
-                newNode->m_infoset->m_actions.end(), [this, newNode](const GameActionRep *) {
+                newNode->m_infoset->m_actions.end(),
+                [this, newNode](std::shared_ptr<GameActionRep>) {
                   newNode->m_children.push_back(new GameNodeRep(this, newNode));
                 });
 

src/pygambit/action.pxi

@@ -19,6 +19,9 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
 #
+import cython
+from cython.operator cimport dereference
+
 
 @cython.cclass
 class Action:
@@ -48,14 +51,14 @@ class Action:
         )
 
     def __hash__(self) -> int:
-        return cython.cast(cython.long, self.action.deref())
+        return cython.cast(cython.long, self.action.deref().get())
 
     @property
     def number(self) -> int:
         """Returns the number of the action at its information set.
         Actions are numbered starting with 0.
         """
-        return self.action.deref().GetNumber() - 1
+        return dereference(self.action.deref()).GetNumber() - 1
 
     def precedes(self, node: Node) -> bool:
         """Returns whether `node` precedes this action in the
@@ -68,21 +71,21 @@ class Action:
         """
         if self.infoset.game != node.game:
             raise MismatchError("precedes() requires a node from the same game as the action")
-        return self.action.deref().Precedes(cython.cast(Node, node).node)
+        return dereference(self.action.deref()).Precedes(cython.cast(Node, node).node)
 
     @property
     def label(self) -> str:
         """Get or set the text label of the action."""
-        return self.action.deref().GetLabel().decode("ascii")
+        return dereference(self.action.deref()).GetLabel().decode("ascii")
 
     @label.setter
     def label(self, value: str) -> None:
-        self.action.deref().SetLabel(value.encode("ascii"))
+        dereference(self.action.deref()).SetLabel(value.encode("ascii"))
 
     @property
     def infoset(self) -> Infoset:
         """Get the information set to which the action belongs."""
-        return Infoset.wrap(self.action.deref().GetInfoset())
+        return Infoset.wrap(dereference(self.action.deref()).GetInfoset())
 
     @property
     def prob(self) -> typing.Union[decimal.Decimal, Rational]:
@@ -100,7 +103,7 @@ class Action:
             )
         py_string = cython.cast(
             string,
-            self.action.deref().GetInfoset().deref().GetActionProb(self.action)
+            dereference(self.action.deref()).GetInfoset().deref().GetActionProb(self.action)
         )
         if "." in py_string.decode("ascii"):
             return decimal.Decimal(py_string.decode("ascii"))

src/pygambit/gambit.pxd

@@ -71,9 +71,9 @@ cdef extern from "games/game.h":
         bool operator !=(c_GameNode) except +
         c_GameNodeRep *deref "operator->"() except +RuntimeError
 
-    cdef cppclass c_GameAction "GameObjectPtr<GameActionRep>":
+    cdef cppclass c_GameAction "GameObjectSharedPtr<GameActionRep>":
         bool operator !=(c_GameAction) except +
-        c_GameActionRep *deref "operator->"() except +RuntimeError
+        shared_ptr[c_GameActionRep] deref "operator->"() except +RuntimeError
 
     cdef cppclass c_GameInfoset "GameObjectPtr<GameInfosetRep>":
         bool operator ==(c_GameInfoset) except +