From 009880e0111ee45ad0b748fd62015917b8fbd4df Mon Sep 17 00:00:00 2001
From: Martin Gieseking <martin.gieseking@uos.de>
Date: Fri, 27 Oct 2017 19:00:13 +0200
Subject: [PATCH] manage heap memory in DependencyGraph with unique_ptrs

---
 src/DependencyGraph.hpp       | 56 +++++++++++++++++------------------
 src/SVGTree.cpp               | 12 ++++----
 tests/DependencyGraphTest.cpp |  3 +-
 3 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/src/DependencyGraph.hpp b/src/DependencyGraph.hpp
index f7c7b27f..05b5d1b1 100644
--- a/src/DependencyGraph.hpp
+++ b/src/DependencyGraph.hpp
@@ -22,15 +22,16 @@
 #define DEPENDENCYGRAPH_HPP
 
 #include <map>
+#include <memory>
 #include <vector>
+#include "utility.hpp"
 
 template <typename T>
-class DependencyGraph
-{
+class DependencyGraph {
 	struct GraphNode {
 		using Dependees = typename std::vector<GraphNode*>;
 
-		GraphNode (const T &k) : key(k), dependent(0) {}
+		GraphNode (const T &k) : key(k), dependent() {}
 
 		void addDependee (GraphNode *node) {
 			if (node) {
@@ -39,12 +40,9 @@ class DependencyGraph
 			}
 		}
 
-		void deleteDependentsAndSelf () {
-			if (dependent)
-				dependent->deleteDependentsAndSelf();
-			for (typename Dependees::iterator it = dependees.begin(); it != dependees.end(); ++it)
-				(*it)->dependent = 0;
-			delete this;
+		void unlinkDependees () {
+			for (GraphNode *dependee : dependees)
+				dependee->dependent = nullptr;
 		}
 
 		T key;
@@ -52,44 +50,46 @@ class DependencyGraph
 		Dependees dependees;
 	};
 
-	using NodeMap = std::map<T, GraphNode*>;
+	using NodeMap = std::map<T, std::unique_ptr<GraphNode>>;
 
 	public:
-		~DependencyGraph() {
-			for (auto &nmpair : _nodeMap)
-				delete nmpair.second;
-		}
-
+		/** Inserts a new isolated node into the dependency graph. */
 		void insert (const T &key) {
 			if (!contains(key))
-				_nodeMap[key] = new GraphNode(key);
+				_nodeMap.emplace(key, util::make_unique<GraphNode>(key));
 		}
 
-		void insert (const T &depKey, const T &key) {
-			if (contains(key))
-				return;
-			auto it = _nodeMap.find(depKey);
-			if (it != _nodeMap.end()) {
-				GraphNode *node = new GraphNode(key);
-				it->second->addDependee(node);
-				_nodeMap[key] = node;
+		/** Inserts a new node to the graph and adds a dependency on an existing one to it.
+		 *  @param[in] key ID of new node to insert
+		 *  @param[in] dependantKey ID of node the new node should depend on */
+		void insert (const T &dependentKey, const T &key) {
+			if (!contains(key)) {
+				auto dependentIter = _nodeMap.find(dependentKey);
+				if (dependentIter != _nodeMap.end()) {
+					auto node = util::make_unique<GraphNode>(key);
+					dependentIter->second->addDependee(node.get());
+					_nodeMap.emplace(key, std::move(node));
+				}
 			}
 		}
 
+		/** Removes a node and all its dependents from the graph. */
 		void removeDependencyPath (const T &key) {
 			auto it = _nodeMap.find(key);
 			if (it != _nodeMap.end()) {
-				GraphNode *startNode = it->second;
-				for (GraphNode *node=startNode; node; node=node->dependent)
+				GraphNode *startNode = it->second.get();
+				for (GraphNode *node=startNode; node; node=node->dependent) {
+					node->unlinkDependees();
 					_nodeMap.erase(node->key);
-				startNode->deleteDependentsAndSelf();
+				}
 			}
 		}
 
+		/** Returns the IDs of all nodes present in the graph. */
 		std::vector<T> getKeys () const {
 			std::vector<T> keys;
 			for (auto &entry : _nodeMap)
-				keys.push_back(entry.first);
+				keys.emplace_back(entry.first);
 			return keys;
 		}
 
diff --git a/src/SVGTree.cpp b/src/SVGTree.cpp
index 52cd933f..796d7cbf 100644
--- a/src/SVGTree.cpp
+++ b/src/SVGTree.cpp
@@ -324,16 +324,16 @@ static inline string extract_id_from_url (const string &url) {
 }
 
 
-/** Removes elements present in the SVH tree that are not required.
+/** Removes elements present in the SVG tree that are not required.
  *  For now, only clipPath elements are removed. */
 void SVGTree::removeRedundantElements () {
-	vector<XMLElementNode*> clipElements;
-	if (!_defs || !_defs->getDescendants("clipPath", 0, clipElements))
+	vector<XMLElementNode*> clipPathElements;
+	if (!_defs || !_defs->getDescendants("clipPath", nullptr, clipPathElements))
 		return;
 
 	// collect dependencies between clipPath elements in the defs section of the SVG tree
 	DependencyGraph<string> idTree;
-	for (const XMLElementNode *clip : clipElements) {
+	for (const XMLElementNode *clip : clipPathElements) {
 		if (const char *id = clip->getAttributeValue("id")) {
 			if (const char *url = clip->getAttributeValue("clip-path"))
 				idTree.insert(extract_id_from_url(url), id);
@@ -341,9 +341,9 @@ void SVGTree::removeRedundantElements () {
 				idTree.insert(id);
 		}
 	}
-	// collect elements that reference a clipPath (have a clip-path attribute)
+	// collect elements that reference a clipPath, i.e. have a clip-path attribute
 	vector<XMLElementNode*> descendants;
-	_page->getDescendants(0, "clip-path", descendants);
+	_page->getDescendants(nullptr, "clip-path", descendants);
 	// remove referenced IDs and their dependencies from the dependency graph
 	for (const XMLElementNode *elem : descendants) {
 		string idref = extract_id_from_url(elem->getAttributeValue("clip-path"));
diff --git a/tests/DependencyGraphTest.cpp b/tests/DependencyGraphTest.cpp
index 5f0ce738..7c633788 100644
--- a/tests/DependencyGraphTest.cpp
+++ b/tests/DependencyGraphTest.cpp
@@ -49,8 +49,9 @@ TEST(DependencyGraphTest, getKeys) {
 TEST(DependencyGraphTest, insert) {
 	DependencyGraph<int> graph;
 	populate(graph);
-	for (int i=1; i <= 7; i++)
+	for (int i=1; i <= 7; i++) {
 		ASSERT_TRUE(graph.contains(i));
+	}
 	ASSERT_FALSE(graph.contains(0));
 	ASSERT_FALSE(graph.contains(8));
 }