data: removed Singleton from Dictionary and made it a member of SearchIndex

Dictionary never had a good reason of being a Singleton in the first place so this will avoid concurrency and
structural issues in the future.

src/lib/data/SearchIndex.cpp

@@ -5,7 +5,6 @@
 
 #include "data/query/QueryToken.h"
 #include "utility/logging/logging.h"
-#include "utility/text/Dictionary.h"
 #include "utility/utilityString.h"
 
 namespace
@@ -131,21 +130,22 @@ std::deque<SearchIndex::SearchNode*> SearchIndex::SearchNode::getParentsWithoutT
 	return nodes;
 }
 
-std::shared_ptr<SearchIndex::SearchNode> SearchIndex::SearchNode::addNodeRecursive(std::deque<Id>* nameIds)
+std::shared_ptr<SearchIndex::SearchNode> SearchIndex::SearchNode::addNodeRecursive(
-{
+	std::deque<Id>* nameIds, const Dictionary& dictionary
+){
 	Id nameId = nameIds->front();
 	nameIds->pop_front();
 
 	std::shared_ptr<SearchNode> node = getChildWithNameId(nameId);
 	if (!node)
 	{
-		node = std::make_shared<SearchNode>(this, Dictionary::getInstance()->getWord(nameId), nameId);
+		node = std::make_shared<SearchNode>(this, dictionary.getWord(nameId), nameId);
 		m_nodes.insert(node);
 	}
 
 	if (nameIds->size())
 	{
-		return node->addNodeRecursive(nameIds);
+		return node->addNodeRecursive(nameIds, dictionary);
 	}
 
 	return node;
@@ -374,13 +374,23 @@ void SearchIndex::clear()
 	m_root.m_nodes.clear();
 }
 
+Id SearchIndex::getWordId(const std::string& word)
+{
+	return m_dictionary.getWordId(word);
+}
+
+const std::string& SearchIndex::getWord(Id wordId) const
+{
+	return m_dictionary.getWord(wordId);
+}
+
 SearchIndex::SearchNode* SearchIndex::addNode(const std::string& fullName)
 {
-	std::deque<Id> nameIds = Dictionary::getInstance()->getWordIds(fullName, DELIMITER);
+	std::deque<Id> nameIds = m_dictionary.getWordIds(fullName, DELIMITER);
 
 	if (nameIds.size())
 	{
-		return m_root.addNodeRecursive(&nameIds).get();
+		return m_root.addNodeRecursive(&nameIds, m_dictionary).get();
 	}
 
 	return nullptr;
@@ -388,7 +398,7 @@ SearchIndex::SearchNode* SearchIndex::addNode(const std::string& fullName)
 
 SearchIndex::SearchNode* SearchIndex::getNode(const std::string& fullName) const
 {
-	std::deque<Id> nameIds = Dictionary::getInstance()->getWordIds(fullName, DELIMITER);
+	std::deque<Id> nameIds = m_dictionary.getWordIdsConst(fullName, DELIMITER);
 
 	if (nameIds.size())
 	{

src/lib/data/SearchIndex.h

@@ -8,6 +8,7 @@
 #include <set>
 #include <vector>
 
+#include "utility/text/Dictionary.h"
 #include "utility/types.h"
 
 class SearchIndex
@@ -54,7 +55,7 @@ public:
 
 	private:
 		// Accessed by SearchIndex
-		std::shared_ptr<SearchNode> addNodeRecursive(std::deque<Id>* nameIds);
+		std::shared_ptr<SearchNode> addNodeRecursive(std::deque<Id>* nameIds, const Dictionary& dictionary);
 		std::shared_ptr<SearchNode> getNodeRecursive(std::deque<Id>* nameIds) const;
 		std::vector<SearchIndex::SearchMatch> findFuzzyMatches(const std::string& query) const;
 
@@ -84,6 +85,9 @@ public:
 
 	void clear();
 
+	Id getWordId(const std::string& word);
+	const std::string& getWord(Id wordId) const;
+
 	SearchNode* addNode(const std::string& fullName);
 	SearchNode* getNode(const std::string& fullName) const;
 
@@ -93,6 +97,7 @@ public:
 
 private:
 	SearchNode m_root;
+	Dictionary m_dictionary;
 };
 
 #endif // SEARCH_INDEX_H

src/lib/data/Storage.cpp

@@ -535,22 +535,14 @@ const Graph& Storage::getGraph() const
 	return m_graph;
 }
 
-Token* Storage::getTokenWithId(Id tokenId) const
+const TokenLocationCollection& Storage::getTokenLocationCollection() const
 {
-	return m_graph.getTokenById(tokenId);
+	return m_locationCollection;
 }
 
-std::vector<TokenLocation*> Storage::getTokenLocationsForId(Id tokenId) const
+const SearchIndex& Storage::getSearchIndex() const
 {
-	const std::vector<Id>& locationIds = getTokenWithId(tokenId)->getLocationIds();
+	return m_index;
-
-	std::vector<TokenLocation*> result;
-	for (Id locationId : locationIds)
-	{
-		result.push_back(m_locationCollection.findTokenLocationById(locationId));
-	}
-
-	return result;
 }
 
 void Storage::initSearchIndex()
@@ -582,7 +574,11 @@ Node* Storage::addNodeHierarchyWithDistinctSignature(Node::NodeType type, const
 		return nullptr;
 	}
 
-	return m_graph.createNodeHierarchyWithDistinctSignature(type, searchNode, ParserClient::functionSignatureStr(function));
+	// TODO: Instead of saving the whole signature string, the signature should be just a set of wordIds.
+	Id signatureId = m_index.getWordId(ParserClient::functionSignatureStr(function));
+	std::shared_ptr<TokenComponentSignature> signature = std::make_shared<TokenComponentSignature>(signatureId);
+
+	return m_graph.createNodeHierarchyWithDistinctSignature(type, searchNode, signature);
 }
 
 TokenComponentAccess::AccessType Storage::convertAccessType(ParserClient::AccessType access) const

src/lib/data/Storage.h

@@ -85,8 +85,8 @@ public:
 
 protected:
 	const Graph& getGraph() const;
-	Token* getTokenWithId(Id tokenId) const;
+	const TokenLocationCollection& getTokenLocationCollection() const;
-	std::vector<TokenLocation*> getTokenLocationsForId(Id tokenId) const;
+	const SearchIndex& getSearchIndex() const;
 
 private:
 	void initSearchIndex();

src/lib/data/graph/StorageGraph.cpp

@@ -1,7 +1,6 @@
 #include "data/graph/StorageGraph.h"
 
 #include "data/graph/token_component/TokenComponentName.h"
-#include "data/graph/token_component/TokenComponentSignature.h"
 #include "utility/logging/logging.h"
 #include "utility/utilityString.h"
 
@@ -30,11 +29,9 @@ Node* StorageGraph::createNodeHierarchy(Node::NodeType type, SearchIndex::Search
 }
 
 Node* StorageGraph::createNodeHierarchyWithDistinctSignature(
-	Node::NodeType type, SearchIndex::SearchNode* searchNode, const std::string& signature
+	Node::NodeType type, SearchIndex::SearchNode* searchNode, std::shared_ptr<TokenComponentSignature> signature
 ){
 	Node* node = getNodeById(searchNode->getFirstTokenId());
-	std::shared_ptr<TokenComponentSignature> sigPtr = TokenComponentSignature::create(signature);
-
 	if (!node)
 	{
 		node = insertNodeHierarchy(type, searchNode);
@@ -42,10 +39,10 @@ Node* StorageGraph::createNodeHierarchyWithDistinctSignature(
 	else
 	{
 		std::function<bool(Node*)> findSignature =
-			[sigPtr](Node* n)
+			[signature](Node* n)
 			{
-				TokenComponentSignature* c = n->getComponent<TokenComponentSignature>();
+				TokenComponentSignature* sig = n->getComponent<TokenComponentSignature>();
-				return c && *c == *sigPtr.get();
+				return sig && *sig == *signature.get();
 			};
 
 		Node* parentNode = node->getParentNode();
@@ -72,7 +69,7 @@ Node* StorageGraph::createNodeHierarchyWithDistinctSignature(
 		}
 	}
 
-	node->addComponentSignature(sigPtr);
+	node->addComponentSignature(signature);
 	return node;
 }
 

src/lib/data/graph/StorageGraph.h

@@ -2,6 +2,7 @@
 #define STORAGE_GRAPH_H
 
 #include "data/graph/Graph.h"
+#include "data/graph/token_component/TokenComponentSignature.h"
 #include "data/SearchIndex.h"
 
 class StorageGraph
@@ -13,7 +14,7 @@ public:
 
 	Node* createNodeHierarchy(Node::NodeType type, SearchIndex::SearchNode* searchNode);
 	Node* createNodeHierarchyWithDistinctSignature(
-		Node::NodeType type, SearchIndex::SearchNode* searchNode, const std::string& signature);
+		Node::NodeType type, SearchIndex::SearchNode* searchNode, std::shared_ptr<TokenComponentSignature> signature);
 	Edge* createEdge(Edge::EdgeType type, Node* from, Node* to);
 
 private:

src/lib/data/graph/token_component/TokenComponentSignature.cpp

@@ -1,11 +1,8 @@
 #include "data/graph/token_component/TokenComponentSignature.h"
 
-#include "utility/text/Dictionary.h"
+TokenComponentSignature::TokenComponentSignature(Id wordId)
-
+	: m_wordId(wordId)
-std::shared_ptr<TokenComponentSignature> TokenComponentSignature::create(const std::string& signature)
 {
-	return std::shared_ptr<TokenComponentSignature>(
-		new TokenComponentSignature(Dictionary::getInstance()->getWordId(signature)));
 }
 
 TokenComponentSignature::~TokenComponentSignature()
@@ -17,17 +14,12 @@ std::shared_ptr<TokenComponent> TokenComponentSignature::copy() const
 	return std::make_shared<TokenComponentSignature>(*this);
 }
 
-const std::string& TokenComponentSignature::getSignature() const
+Id TokenComponentSignature::getWordId() const
 {
-	return Dictionary::getInstance()->getWord(m_wordId);
+	return m_wordId;
 }
 
 bool TokenComponentSignature::operator==(const TokenComponentSignature& other) const
 {
 	return m_wordId == other.m_wordId;
 }
-
-TokenComponentSignature::TokenComponentSignature(Id wordId)
-	: m_wordId(wordId)
-{
-}

src/lib/data/graph/token_component/TokenComponentSignature.h

@@ -1,9 +1,6 @@
 #ifndef TOKEN_COMPONENT_SIGNATURE_H
 #define TOKEN_COMPONENT_SIGNATURE_H
 
-#include <memory>
-#include <string>
-
 #include "data/graph/token_component/TokenComponent.h"
 #include "utility/types.h"
 
@@ -11,19 +8,16 @@ class TokenComponentSignature
 	: public TokenComponent
 {
 public:
-	static std::shared_ptr<TokenComponentSignature> create(const std::string& signature);
+	TokenComponentSignature(Id wordId);
-
 	virtual ~TokenComponentSignature();
 
 	virtual std::shared_ptr<TokenComponent> copy() const;
 
-	const std::string& getSignature() const;
+	Id getWordId() const;
 
 	bool operator==(const TokenComponentSignature& other) const;
 
 private:
-	TokenComponentSignature(Id wordId);
-
 	const Id m_wordId;
 };
 

src/lib/utility/text/Dictionary.cpp

@@ -2,22 +2,34 @@
 
 #include "utility/utilityString.h"
 
-std::shared_ptr<Dictionary> Dictionary::getInstance()
+Dictionary::Dictionary()
 {
-	std::lock_guard<std::mutex> lockGuard(s_instanceMutex);
-	if (!s_instance)
-	{
-		s_instance = std::shared_ptr<Dictionary>(new Dictionary());
-	}
-	return s_instance;
 }
 
 Dictionary::~Dictionary()
 {
 }
 
+void Dictionary::clear()
+{
+	m_words.clear();
+}
+
 Id Dictionary::getWordId(const std::string& word)
 {
+	Id wordId = getWordIdConst(word);
+	if (wordId)
+	{
+		return wordId;
+	}
+
+	m_words.emplace(++s_nextId, word);
+	return s_nextId;
+}
+
+Id Dictionary::getWordIdConst(const std::string& word) const
+{
+	// TODO: This word lookup is very inefficient, use something smarter like a trie.
 	for (std::unordered_map<Id, std::string>::const_iterator it = m_words.begin(); it != m_words.end(); it++)
 	{
 		if (it->second == word)
@@ -26,8 +38,7 @@ Id Dictionary::getWordId(const std::string& word)
 		}
 	}
 
-	m_words.emplace(++s_nextId, word);
+	return 0;
-	return s_nextId;
 }
 
 std::deque<Id> Dictionary::getWordIds(const std::string& wordList, const std::string& delimiter)
@@ -43,6 +54,19 @@ std::deque<Id> Dictionary::getWordIds(const std::string& wordList, const std::st
 	return ids;
 }
 
+std::deque<Id> Dictionary::getWordIdsConst(const std::string& wordList, const std::string& delimiter) const
+{
+	std::deque<std::string> words = utility::split(wordList, delimiter);
+	std::deque<Id> ids;
+
+	for (const std::string& word: words)
+	{
+		ids.push_back(getWordIdConst(word));
+	}
+
+	return ids;
+}
+
 const std::string& Dictionary::getWord(Id id) const
 {
 	std::unordered_map<Id, std::string>::const_iterator it = m_words.find(id);
@@ -72,10 +96,4 @@ std::string Dictionary::getWord(const std::deque<Id> ids, const std::string& del
 	return word;
 }
 
-Dictionary::Dictionary()
-{
-}
-
-std::shared_ptr<Dictionary> Dictionary::s_instance;
-std::mutex Dictionary::s_instanceMutex;
 Id Dictionary::s_nextId = 0;

src/lib/utility/text/Dictionary.h

@@ -12,23 +12,22 @@
 class Dictionary
 {
 public:
-	static std::shared_ptr<Dictionary> getInstance();
+	Dictionary();
 	~Dictionary();
 
+	void clear();
+
 	Id getWordId(const std::string& word);
+	Id getWordIdConst(const std::string& word) const;
+
 	std::deque<Id> getWordIds(const std::string& wordList, const std::string& delimiter);
+	std::deque<Id> getWordIdsConst(const std::string& wordList, const std::string& delimiter) const;
 
 	// Note: References to values in an unordered_map don't change on rehashing so they can be saved and used elsewhere.
 	const std::string& getWord(Id id) const;
 	std::string getWord(const std::deque<Id> ids, const std::string& delimiter) const;
 
 private:
-	Dictionary();
-	Dictionary(const Dictionary&);
-	void operator=(const Dictionary&);
-
-	static std::shared_ptr<Dictionary> s_instance;
-	static std::mutex s_instanceMutex;
 	static Id s_nextId;
 
 	std::unordered_map<Id, std::string> m_words;

src/test/DictionaryTestSuite.h

@@ -5,44 +5,43 @@
 class DictionaryTestSuite: public CxxTest::TestSuite
 {
 public:
-	void test_get_instance()
-	{
-		TS_ASSERT(Dictionary::getInstance());
-	}
-
 	void test_does_not_find_unsaved_word()
 	{
-		TS_ASSERT_EQUALS("", Dictionary::getInstance()->getWord(12));
+		Dictionary dictionary;
+		TS_ASSERT_EQUALS("", dictionary.getWord(12));
 	}
 
 	void test_can_save_word_and_retrieve_it_with_id()
 	{
-		Id id = Dictionary::getInstance()->getWordId("hello world!");
+		Dictionary dictionary;
+		Id id = dictionary.getWordId("hello world!");
 
 		TS_ASSERT_LESS_THAN(0, id);
-		TS_ASSERT_EQUALS("hello world!", Dictionary::getInstance()->getWord(id));
+		TS_ASSERT_EQUALS("hello world!", dictionary.getWord(id));
 	}
 
 	void test_can_save_multiple_words_and_retrieve_it_with_id()
 	{
-		std::deque<Id> ids = Dictionary::getInstance()->getWordIds("hello world!", " ");
+		Dictionary dictionary;
+		std::deque<Id> ids = dictionary.getWordIds("hello world!", " ");
 
 		TS_ASSERT_EQUALS(2, ids.size());
-		TS_ASSERT_EQUALS("hello", Dictionary::getInstance()->getWord(ids.front()));
+		TS_ASSERT_EQUALS("hello", dictionary.getWord(ids.front()));
-		TS_ASSERT_EQUALS("world!", Dictionary::getInstance()->getWord(ids.back()));
+		TS_ASSERT_EQUALS("world!", dictionary.getWord(ids.back()));
-		TS_ASSERT_EQUALS("hello world!", Dictionary::getInstance()->getWord(ids, " "));
+		TS_ASSERT_EQUALS("hello world!", dictionary.getWord(ids, " "));
 	}
 
 	void test_next_word_gets_different_id()
 	{
-		Id id = Dictionary::getInstance()->getWordId("hello world!");
+		Dictionary dictionary;
-		Id id2 = Dictionary::getInstance()->getWordId("foobar");
+		Id id = dictionary.getWordId("hello world!");
+		Id id2 = dictionary.getWordId("foobar");
 
 		TS_ASSERT_LESS_THAN(0, id);
 		TS_ASSERT_LESS_THAN(0, id2);
 		TS_ASSERT_LESS_THAN(id, id2);
 
-		TS_ASSERT_EQUALS("hello world!", Dictionary::getInstance()->getWord(id));
+		TS_ASSERT_EQUALS("hello world!", dictionary.getWord(id));
-		TS_ASSERT_EQUALS("foobar", Dictionary::getInstance()->getWord(id2));
+		TS_ASSERT_EQUALS("foobar", dictionary.getWord(id2));
 	}
 };

src/test/StorageGraphTestSuite.h

@@ -167,16 +167,16 @@ public:
 	void test_graph_saves_nodes_with_distinct_signatures()
 	{
 		TestStorageGraph graph;
-		Node* a1 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_FUNCTION, "A", "A1");
+		Node* a1 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_FUNCTION, "A", 1);
-		Node* a2 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_FUNCTION, "A", "A2");
+		Node* a2 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_FUNCTION, "A", 2);
-		Node* a3 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_FUNCTION, "A", "A2");
+		Node* a3 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_FUNCTION, "A", 2);
 
 		TS_ASSERT_DIFFERS(a1, a2);
 		TS_ASSERT_EQUALS(a2, a3);
 
-		Node* c1 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_METHOD, "B::C", "C1");
+		Node* c1 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_METHOD, "B::C", 3);
-		Node* c2 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_METHOD, "B::C", "C2");
+		Node* c2 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_METHOD, "B::C", 4);
-		Node* c3 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_METHOD, "B::C", "C2");
+		Node* c3 = graph.createNodeHierarchyWithDistinctSignature(Node::NODE_METHOD, "B::C", 4);
 
 		TS_ASSERT_DIFFERS(c1, c2);
 		TS_ASSERT_EQUALS(c2, c3);
@@ -300,9 +300,10 @@ private:
 		}
 
 		Node* createNodeHierarchyWithDistinctSignature(
-			Node::NodeType type, const std::string& name, const std::string& signature
+			Node::NodeType type, const std::string& name, Id signatureId
 		){
 			SearchIndex::SearchNode* searchNode = m_index.addNode(name);
+			std::shared_ptr<TokenComponentSignature> signature = std::make_shared<TokenComponentSignature>(signatureId);
 			return StorageGraph::createNodeHierarchyWithDistinctSignature(type, searchNode, signature);
 		}
 

src/test/StorageTestSuite.h

@@ -180,7 +180,7 @@ public:
 		TS_ASSERT_EQUALS(paramEdge->getTo()->getFullName(), "char");
 
 		TS_ASSERT(node->getComponent<TokenComponentSignature>());
-		TS_ASSERT_EQUALS(node->getComponent<TokenComponentSignature>()->getSignature(), "isTrue(char)");
+		TS_ASSERT_EQUALS(storage.getWord(node->getComponent<TokenComponentSignature>()->getWordId()), "isTrue(char)");
 
 		std::vector<TokenLocation*> locations = storage.getLocationsForId(id);
 		TS_ASSERT_EQUALS(locations.size(), 2);
@@ -214,7 +214,7 @@ public:
 		TS_ASSERT_EQUALS(paramEdge->getTo()->getFullName(), "bool");
 
 		TS_ASSERT(node->getComponent<TokenComponentSignature>());
-		TS_ASSERT_EQUALS(node->getComponent<TokenComponentSignature>()->getSignature(), "isMethod(bool)");
+		TS_ASSERT_EQUALS(storage.getWord(node->getComponent<TokenComponentSignature>()->getWordId()), "isMethod(bool)");
 
 		std::vector<TokenLocation*> locations = storage.getLocationsForId(id);
 		TS_ASSERT_EQUALS(locations.size(), 2);
@@ -511,17 +511,30 @@ private:
 	public:
 		Node* getNodeWithId(Id id) const
 		{
-			return dynamic_cast<Node*>(getTokenWithId(id));
+			return dynamic_cast<Node*>(getGraph().getTokenById(id));
 		}
 
 		Edge* getEdgeWithId(Id id) const
 		{
-			return dynamic_cast<Edge*>(getTokenWithId(id));
+			return dynamic_cast<Edge*>(getGraph().getTokenById(id));
 		}
 
 		std::vector<TokenLocation*> getLocationsForId(Id id) const
 		{
-			return getTokenLocationsForId(id);
+			const std::vector<Id>& locationIds = getGraph().getTokenById(id)->getLocationIds();
+
+			std::vector<TokenLocation*> result;
+			for (Id locationId : locationIds)
+			{
+				result.push_back(getTokenLocationCollection().findTokenLocationById(locationId));
+			}
+
+			return result;
+		}
+
+		const std::string& getWord(Id wordId) const
+		{
+			return getSearchIndex().getWord(wordId);
 		}
 	};