Manage nodes with shared_ptr

GraphVisitor.cpp

@@ -36,7 +36,7 @@ void GraphVisitor::visit(Graph * graph) {
     }
 
     for (int i=0; i < graph->numNodes(); i++) {
-        NodeBase * node = graph->node(i);
+        std::shared_ptr<NodeBase> node = graph->node(i);
         onNode(node);
 
         for (int j=0; j < node->numProperty(); j++) {

GraphVisitor.h

@@ -40,10 +40,10 @@ namespace media_graph {
         void visit(Graph * graph);
 
     protected:
-        virtual void onNode(NodeBase * node) {}
-        virtual void onStream(NodeBase * node, NamedStream * stream) {}
-        virtual void onPin(NodeBase * node, NamedPin * pin) {}
-        virtual void onProperty(NodeBase * node, NamedStream * stream, NamedPin * pin, NamedProperty * prop) {}  
+        virtual void onNode(std::shared_ptr<NodeBase> node) {}
+        virtual void onStream(std::shared_ptr<NodeBase> node, NamedStream * stream) {}
+        virtual void onPin(std::shared_ptr<NodeBase> node, NamedPin * pin) {}
+        virtual void onProperty(std::shared_ptr<NodeBase> node, NamedStream * stream, NamedPin * pin, NamedProperty * prop) {}  
 
     };
 

GraphVisitor_test.cpp

@@ -40,8 +40,7 @@ namespace {
 
 class NodeWithProperty : public NodeBase {
 public:
-    NodeWithProperty (const std::string& wanted_name, Graph* graph)
-        : NodeBase(wanted_name, graph), A("int A", 1) { }
+    NodeWithProperty() : A("int A", 1) { }
 
     virtual int numProperty() const { return 1 + PropertyList::numProperty(); }
     virtual NamedProperty* property(int id) {
@@ -67,20 +66,20 @@ class GraphAnalyser: GraphVisitor {
     }
 
 protected:
-    virtual void onNode(NodeBase * node) {
+    virtual void onNode(std::shared_ptr<NodeBase> node) override {
         EXPECT_EQ("node prop int",node->name());
         nodeCount++;
     }
 
-    virtual void onStream(NodeBase * node, NamedStream * stream) {
+    virtual void onStream(std::shared_ptr<NodeBase> node, NamedStream * stream) override {
         EXPECT_TRUE(false) << "no stream in graph-should not pass here";
     }
 
-    virtual void onPin(NodeBase * node, NamedPin * pin) {
+    virtual void onPin(std::shared_ptr<NodeBase> node, NamedPin * pin) override {
         EXPECT_TRUE(false) << "no pin in graph-should not pass here";
     }
 
-    virtual void onProperty(NodeBase * node, NamedStream * stream, NamedPin * pin, NamedProperty * prop) {
+    virtual void onProperty(std::shared_ptr<NodeBase> node, NamedStream * stream, NamedPin * pin, NamedProperty * prop) override {
         if (node==0) {
             //test on graph property
             EXPECT_EQ("started",prop->name());
@@ -100,7 +99,7 @@ class GraphAnalyser: GraphVisitor {
 TEST(GraphVisitor, parseGraph) {
 
     Graph graph;
-    NodeWithProperty * node  = new NodeWithProperty("node prop int", &graph);
+    std::shared_ptr<NodeWithProperty> node  = graph.newNode<NodeWithProperty>("node prop int");
 
     GraphAnalyser grfAnal;
 

graph.cpp

@@ -40,40 +40,46 @@ Graph::Graph() : started_(false) {
     addGetProperty("started", this, &Graph::isStarted);
 }
 
-bool Graph::addNode(const std::string& name, NodeBase* node) {
+bool Graph::addNode(const std::string& name, std::shared_ptr<NodeBase> node) {
     ScopedLock lock(&mutex_);
-    assert(node != 0);
-    if (lockedGetNodeByName(name) != 0) {
+    assert(node);
+    if (lockedGetNodeByName(name)) {
         // We do not accept two nodes with the same name.
         return false;
     }
 
     nodes_[name] = node;
+    node->setNameAndGraph(name, this);
     return true;
 }
 
-void Graph::removeNode(NodeBase* node) {
+void Graph::removeNode(const std::string& name) {
     ScopedLock lock(&mutex_);
 
-    std::map<string, NodeBase*>::iterator it = nodes_.find(node->name());
+    auto it = nodes_.find(name);
     if (it != nodes_.end()) {
-        it->second->disconnectAllPins();
-        it->second->disconnectAllStreams();
+        auto node = it->second;
         nodes_.erase(it);
+
+        node->disconnectAllPins();
+        node->disconnectAllStreams();
+
+        // Make sure to unlock before "node" is destroyed.
+        lock.unlock();
     }
 }
 
-NodeBase* Graph::getNodeByName(const std::string& name) {
+std::shared_ptr<NodeBase> Graph::getNodeByName(const std::string& name) {
     ScopedLock lock(&mutex_);
     return lockedGetNodeByName(name);
 }
 
-NodeBase* Graph::lockedGetNodeByName(const std::string& name) {
-    std::map<string, NodeBase*>::iterator it = nodes_.find(name);
+std::shared_ptr<NodeBase> Graph::lockedGetNodeByName(const std::string& name) {
+    auto it = nodes_.find(name);
     if (it != nodes_.end()) {
         return it->second;
     }
-    return 0;
+    return std::shared_ptr<NodeBase>();
 }
 
 bool Graph::start() {
@@ -82,7 +88,7 @@ bool Graph::start() {
     }
 
     ScopedLock lock(&mutex_);
-    for (std::map<string, NodeBase*>::iterator it = nodes_.begin();
+    for (auto it = nodes_.begin();
          it != nodes_.end(); ++it) {
         if (!it->second->start()) {
             // TODO: give a meaningful error.
@@ -100,7 +106,7 @@ void Graph::stop() {
 }
 
 void Graph::lockedStop() {
-    for (std::map<string, NodeBase*>::iterator it = nodes_.begin();
+    for (auto it = nodes_.begin();
          it != nodes_.end(); ++it) {
         it->second->closeConnectedPins();
         it->second->stop();
@@ -110,8 +116,8 @@ void Graph::lockedStop() {
 void Graph::clear() {
     stop();
 
-    while (nodes_.size() > 0) {
-        delete nodes_.begin()->second;
+    while (nodes_.size()) {
+        removeNode(nodes_.begin()->first);
     }
 }
 
@@ -121,14 +127,16 @@ bool Graph::connect(NamedStream* stream, NamedPin* pin) {
     }
 
     // We allow streams and pins not belonging to any node.
-    assert(stream->node() == 0 || stream->node()->graph() == this);
-    assert(pin->node() == 0 || pin->node()->graph() == this);
+    assert(!stream->node() || stream->node()->graph() == this);
+    assert(!pin->node() || pin->node()->graph() == this);
 
     return pin->connect(stream);
 }
 
-bool Graph::connect(NodeBase* source, const std::string& streamName,
-                    NodeBase* dest, const std::string& pinName) {
+bool Graph::connect(std::shared_ptr<NodeBase> source,
+                    const std::string& streamName,
+                    std::shared_ptr<NodeBase> dest,
+                    const std::string& pinName) {
     if (!source || !dest) {
         return false;
     }
@@ -143,11 +151,11 @@ bool Graph::connect(const std::string& source_name, const std::string& streamNam
                    getNodeByName(dest_name), pinName);
 }
 
-NodeBase* Graph::node(int num) const {
-    std::map<std::string, NodeBase*>::const_iterator it = nodes_.begin();
+std::shared_ptr<NodeBase> Graph::node(int num) const {
+    auto it = nodes_.begin();
     for (int i = 0; i < num; ++i) {
         if (it == nodes_.end()) {
-            return 0;
+            return nullptr;
         }
         ++it;
     }

graph.h

@@ -24,7 +24,7 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 // SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 //
-// Julien.Pilet@aptarism.com, 2012.
+// Julien.Pilet@gmail.com, 2012, 2020.
 #ifndef MEDIAGRAPH_GRAPH_H
 #define MEDIAGRAPH_GRAPH_H
 
@@ -34,6 +34,8 @@
 
 #include <string>
 #include <map>
+#include <memory>
+#include <sstream>
 
 namespace media_graph {
 
@@ -69,11 +71,11 @@ class Graph : public PropertyList {
     Graph();
     ~Graph() { clear(); }
 
-    /*! Adds a node to the graph. The graph takes ownership of <node> and will
-     *  delete it when clear() is called or on graph destruction.
-     *
-     *  In normal circumstances, addNode should only be called by the NodeBase
-     *  constructor.
+    /// Construct a new node, add it to the graph, and returns a shared_ptr.
+    template<typename T, typename ... Args>
+    std::shared_ptr<T> newNode(const std::string& wanted_name, Args && ... args);
+
+    /*! Adds a node to the graph.
      *
      *  Once added, a node can be retrieved using its name with getNodeByName.
      *
@@ -83,18 +85,18 @@ class Graph : public PropertyList {
      *  Returns true on success. Returns false if a node <name> already exists
      *  in the graph.
      */
-    bool addNode(const std::string& name, NodeBase* node);
+    bool addNode(const std::string& name, std::shared_ptr<NodeBase> node);
 
     /*! Removes a node from the node list. The easiest way to remove a node
      *  in practice is to delete it directly, the destructor will call
      *  removeNode().
      */
-    void removeNode(NodeBase* node);
+    void removeNode(const std::string& name);
 
     /*! Returns the node that was previously added with the given name. If no
      *  matching node is found, returns 0.
      */
-    NodeBase* getNodeByName(const std::string& name);
+    std::shared_ptr<NodeBase> getNodeByName(const std::string& name);
 
     /*! Adds an edge to the graph.
      *  Connects the output stream called <streamName> of source node <source>
@@ -107,8 +109,8 @@ class Graph : public PropertyList {
      */
     bool connect(NamedStream* stream, NamedPin* pin);
 
-    bool connect(NodeBase* source, const std::string& streamName,
-                 NodeBase* dest, const std::string& pinName);
+    bool connect(std::shared_ptr<NodeBase> source, const std::string& streamName,
+                 std::shared_ptr<NodeBase> dest, const std::string& pinName);
 
     //! Adds an edge to the graph.
     bool connect(const std::string& source, const std::string& streamName,
@@ -133,23 +135,37 @@ class Graph : public PropertyList {
 
     int numNodes() const { return nodes_.size(); }
 
-    NodeBase* node(int num) const;
+    std::shared_ptr<NodeBase> node(int num) const;
 
   private:
     // Stop the graph, assumes mutex_ is already aquired.
     void lockedStop();
-    NodeBase* lockedGetNodeByName(const std::string& name);
+    std::shared_ptr<NodeBase> lockedGetNodeByName(const std::string& name);
 
     Graph(const Graph&) { }  // copy constructor is forbidden.
 
 
-    std::map<std::string, NodeBase*> nodes_;
+    std::map<std::string, std::shared_ptr<NodeBase>> nodes_;
     bool started_;
 
     // Protects nodes_ against node addition and removal from multiple threads.
     Mutex mutex_;
 };
 
+
+template<typename T, typename ... Args>
+std::shared_ptr<T> Graph::newNode(const std::string& wanted_name, Args && ... args) {
+    std::shared_ptr<T> ptr = std::make_shared<T>(args...);
+
+    std::string try_name = wanted_name;
+    for (int i = 0; !addNode(try_name, ptr); ++i) {
+        std::ostringstream oss;
+        oss << wanted_name << i;
+        try_name = oss.str();
+    }
+    return ptr;
+}
+
 }  // namespace media_graph
 
 #endif  // MEDIAGRAPH_GRAPH_H