Fix potential unsafe initialization in the Graph class (#606)

* Avoid constructor/destructor fiascos in graph class * Deprecations and migration guide. Signed-off-by: Carlos Agüero <[email protected]> Signed-off-by: Carlos Agüero <[email protected]> Co-authored-by: Steve Peters <[email protected]>
gazebosim · Jul 12, 2024 · 17585a9 · 17585a9
1 parent 1fd7435
commit 17585a9
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 14 deletions.
diff --git a/Migration.md b/Migration.md
@@ -5,6 +5,20 @@ Deprecated code produces compile-time warnings. These warning serve as
 notification to users that their code should be upgraded. The next major
 release will remove the deprecated code.
 
+## Gazebo Math 8.X to 9.X
+
+### Deprecations
+
+1. **graph/Vertex.hh**
+    + The `Vertex::NullVertex` static member is deprecated. Please use
+      `Vertex::NullVertex()` instead.
+      E.g.: https://github.com/gazebosim/gz-math/pull/606/files#diff-0c0220a7e72be70337975433eeddc3f5e072ade5cd80dfb1ac03da233c39c983L153-R153
+
+1. **graph/Edge.hh**
+    + The `Edge::NullEdge` static member is deprecated. Please use
+      `Vertex::NullEdge()` instead.
+      E.g.: https://github.com/gazebosim/gz-math/pull/606/files#diff-0c0220a7e72be70337975433eeddc3f5e072ade5cd80dfb1ac03da233c39c983L222-R222
+
 ## Gazebo Math 7.X to 8.X
 
 ### Breaking Changes

diff --git a/include/gz/math/graph/Edge.hh b/include/gz/math/graph/Edge.hh
@@ -21,6 +21,7 @@
 #include <cstdint>
 #include <functional>
 #include <map>
+#include <memory>
 #include <ostream>
 #include <set>
 
@@ -205,7 +206,8 @@ namespace graph
   class UndirectedEdge : public Edge<E>
   {
     /// \brief An invalid undirected edge.
-    public: static UndirectedEdge<E> NullEdge;
+    // Deprecated in favor of NullEdge().
+    public: static UndirectedEdge<E> GZ_DEPRECATED(8) NullEdge;
 
     /// \brief Constructor.
     /// \param[in] _vertices The vertices of the edge.
@@ -257,6 +259,7 @@ namespace graph
   };
 
   /// \brief An invalid undirected edge.
+  // Deprecated in favor of NullEdge().
   template<typename E>
   UndirectedEdge<E> UndirectedEdge<E>::NullEdge(
     VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);
@@ -268,7 +271,8 @@ namespace graph
   class DirectedEdge : public Edge<E>
   {
     /// \brief An invalid directed edge.
-    public: static DirectedEdge<E> NullEdge;
+    // Deprecated in favor of NullEdge().
+    public: static DirectedEdge<E> GZ_DEPRECATED(8) NullEdge;
 
     /// \brief Constructor.
     /// \param[in] _vertices The vertices of the edge.
@@ -332,9 +336,19 @@ namespace graph
   };
 
   /// \brief An invalid directed edge.
+  // Deprecated in favor of NullEdge().
   template<typename E>
   DirectedEdge<E> DirectedEdge<E>::NullEdge(
     VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);
+
+  /// \brief An invalid edge.
+  template<typename E, typename EdgeType>
+  EdgeType &NullEdge()
+  {
+    static auto e = std::make_unique<EdgeType>(
+      VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);
+    return *e;
+  }
 }
 }
 }

diff --git a/include/gz/math/graph/Graph.hh b/include/gz/math/graph/Graph.hh
@@ -150,7 +150,7 @@ namespace graph
         {
           std::cerr << "[Graph::AddVertex()] The limit of vertices has been "
                     << "reached. Ignoring vertex." << std::endl;
-          return Vertex<V>::NullVertex;
+          return NullVertex<V>();
         }
       }
 
@@ -163,7 +163,7 @@ namespace graph
       {
         std::cerr << "[Graph::AddVertex()] Repeated vertex [" << id << "]"
                   << std::endl;
-        return Vertex<V>::NullVertex;
+        return NullVertex<V>();
       }
 
       // Link the vertex with an empty list of edges.
@@ -219,7 +219,7 @@ namespace graph
       {
         std::cerr << "[Graph::AddEdge()] The limit of edges has been reached. "
                   << "Ignoring edge." << std::endl;
-        return EdgeType::NullEdge;
+        return NullEdge<E, EdgeType>();
       }
 
       EdgeType newEdge(_vertices, _data, _weight, id);
@@ -240,7 +240,7 @@ namespace graph
       for (auto const &v : {edgeVertices.first, edgeVertices.second})
       {
         if (this->vertices.find(v) == this->vertices.end())
-          return EdgeType::NullEdge;
+          return NullEdge<E, EdgeType>();
       }
 
       // Link the new edge.
@@ -611,7 +611,7 @@ namespace graph
     {
       auto iter = this->vertices.find(_id);
       if (iter == this->vertices.end())
-        return Vertex<V>::NullVertex;
+        return NullVertex<V>();
 
       return iter->second;
     }
@@ -624,7 +624,7 @@ namespace graph
     {
       auto iter = this->vertices.find(_id);
       if (iter == this->vertices.end())
-        return Vertex<V>::NullVertex;
+        return NullVertex<V>();
 
       return iter->second;
     }
@@ -646,7 +646,7 @@ namespace graph
 
       // Quit early if there is no adjacency entry
       if (adjIt == this->adjList.end())
-        return EdgeType::NullEdge;
+        return NullEdge<E, EdgeType>();
 
       // Loop over the edges in the source vertex's adjacency list
       for (std::set<EdgeId>::const_iterator edgIt = adjIt->second.begin();
@@ -665,7 +665,7 @@ namespace graph
         }
       }
 
-      return EdgeType::NullEdge;
+      return NullEdge<E, EdgeType>();
     }
 
     /// \brief Get a reference to an edge using its Id.
@@ -676,7 +676,7 @@ namespace graph
     {
       auto iter = this->edges.find(_id);
       if (iter == this->edges.end())
-        return EdgeType::NullEdge;
+        return NullEdge<E, EdgeType>();
 
       return iter->second;
     }
@@ -689,7 +689,7 @@ namespace graph
     {
       auto iter = this->edges.find(_id);
       if (iter == this->edges.end())
-        return EdgeType::NullEdge;
+        return NullEdge<E, EdgeType>();
 
       return iter->second;
     }

diff --git a/include/gz/math/graph/Vertex.hh b/include/gz/math/graph/Vertex.hh
@@ -21,6 +21,7 @@
 #include <cstdint>
 #include <functional>
 #include <map>
+#include <memory>
 #include <ostream>
 #include <string>
 #include <utility>
@@ -45,7 +46,7 @@ namespace graph
   using VertexId_P = std::pair<VertexId, VertexId>;
 
   /// \brief Represents an invalid Id.
-  static const VertexId kNullId = MAX_UI64;
+  constexpr VertexId kNullId = MAX_UI64;
 
   /// \brief A vertex of a graph. It stores user information, an optional name,
   /// and keeps an internal unique Id. This class does not enforce to choose a
@@ -54,7 +55,8 @@ namespace graph
   class Vertex
   {
     /// \brief An invalid vertex.
-    public: static Vertex<V> NullVertex;
+    // Deprecated in favor of NullVertex().
+    public: static Vertex<V> GZ_DEPRECATED(8) NullVertex;
 
     /// \brief Constructor.
     /// \param[in] _name Non-unique vertex name.
@@ -136,9 +138,18 @@ namespace graph
   };
 
   /// \brief An invalid vertex.
+  // Deprecated in favor of NullVertex().
   template<typename V>
   Vertex<V> Vertex<V>::NullVertex("__null__", V(), kNullId);
 
+  /// \brief An invalid vertex.
+  template<typename V>
+  Vertex<V> &NullVertex()
+  {
+    static auto v = std::make_unique<Vertex<V>>("__null__", V(), kNullId);
+    return *v;
+  }
+
   /// \def VertexRef_M
   /// \brief Map of vertices. The key is the vertex Id. The value is a
   /// reference to the vertex.