Improving HashMap interface

include/anki/util/HashMap.h

@@ -6,50 +6,64 @@
 #pragma once
 
 #include "anki/util/Allocator.h"
-#include "anki/util/NonCopyable.h"
-#include "anki/util/Ptr.h"
 #include "anki/util/Functions.h"
+#include "anki/util/NonCopyable.h"
 
 namespace anki {
 
-/// @addtogroup util_private
+/// @addtogroup util_containers
 /// @{
 
-#define ANKI_ENABLE_HASH_MAP(Class_) \
-	template<typename TKey, typename TValue, typename THasher, \
-		typename TCompare, typename TNode> \
-	friend class HashMap; \
-	U64 m_hash = 0; \
-	Class_* m_left = nullptr; \
-	Class_* m_right = nullptr; \
-	Class_* m_parent = nullptr;
+namespace detail {
 
 /// HashMap node. It's not a traditional bucket because it doesn't contain more
 /// than one values.
-template<typename TKey, typename TValue>
+/// @internal
+template<typename TValue>
 class HashMapNode
 {
 public:
 	U64 m_hash;
-	HashMapNode* m_left = nullptr;
-	HashMapNode* m_right = nullptr;
+	HashMapNode* m_left;
+	HashMapNode* m_right;
 	TValue m_value;
-	HashMapNode* m_parent = nullptr;
+	HashMapNode* m_parent; ///< Used for iterating.
 
 	template<typename... TArgs>
 	HashMapNode(TArgs&&... args)
-		: m_value(args...)
+		: m_hash(0)
+		, m_left(nullptr)
+		, m_right(nullptr)
+		, m_value(args...)
+		, m_parent(nullptr)
 	{}
+
+	TValue& getValue()
+	{
+		return m_value;
+	}
+
+	const TValue& getValue() const
+	{
+		return m_value;
+	}
 };
 
 /// HashMap forward-only iterator.
+/// @internal
 template<typename TNodePointer, typename TValuePointer,
 	typename TValueReference>
 class HashMapIterator
 {
-public:
-	TNodePointer m_node;
+	template<typename TKey, typename TValue, typename THasher,
+		typename TCompare>
+	friend class HashMap;
+
+	template<typename TKey, typename TValue, typename THasher,
+		typename TCompare>
+	friend class HashMapAllocFree;
 
+public:
 	/// Default constructor.
 	HashMapIterator()
 		: m_node(nullptr)
@@ -75,13 +89,13 @@ public:
 	TValueReference operator*() const
 	{
 		ANKI_ASSERT(m_node);
-		return m_node->m_value;
+		return m_node->getValue();
 	}
 
 	TValuePointer operator->() const
 	{
 		ANKI_ASSERT(m_node);
-		return &m_node->m_value;
+		return &m_node->getValue();
 	}
 
 	HashMapIterator& operator++()
@@ -104,7 +118,8 @@ public:
 			node = node->m_parent;
 			while(node)
 			{
-				if(node->m_right && node->m_right != prevNode)
+				if(node->m_right
+					&& node->m_right != prevNode)
 				{
 					node = node->m_right;
 					break;
@@ -154,62 +169,36 @@ public:
 	{
 		return !(*this == b);
 	}
-};
-/// @}
 
-/// @addtogroup util_containers
-/// @{
+private:
+	TNodePointer m_node;
+};
 
-/// Hash map template.
+/// Hash map base.
+/// @tparam TKey The key of the map.
+/// @tparam TValue The value of the map.
+/// @internal
 template<typename TKey, typename TValue, typename THasher, typename TCompare,
-	typename TNode = HashMapNode<TKey, TValue>>
-class HashMap: public NonCopyable
+	typename TNode>
+class HashMapBase: public NonCopyable
 {
 public:
 	using Key = TKey;
 	using Value = TValue;
-	using Node = TNode;
 	using Reference = Value&;
 	using ConstReference = const Value&;
 	using Pointer = Value*;
 	using ConstPointer = const Value*;
-	using Iterator = HashMapIterator<Node*, Pointer, Reference>;
+	using Iterator = HashMapIterator<TNode*, Pointer, Reference>;
 	using ConstIterator =
-		HashMapIterator<const Node*, ConstPointer, ConstReference>;
-	enum
-	{
-		T_NODE_SAME_AS_T_VALUE = TypesAreTheSame<Value, Node>::m_value
-	};
+		HashMapIterator<const TNode*, ConstPointer, ConstReference>;
 
 	/// Default constructor.
-	HashMap()
+	HashMapBase()
 		: m_root(nullptr)
 	{}
 
-	/// Move.
-	HashMap(HashMap&& b)
-		: HashMap()
-	{
-		move(b);
-	}
-
-	/// You need to manually destroy the map.
-	/// @see HashMap::destroy
-	~HashMap()
-	{
-		ANKI_ASSERT(m_root == nullptr && "Requires manual destruction");
-	}
-
-	/// Move.
-	HashMap& operator=(HashMap&& b)
-	{
-		move(b);
-		return *this;
-	}
-
-	/// Destroy the list.
-	template<typename TAllocator>
-	void destroy(TAllocator alloc);
+	~HashMapBase() = default;
 
 	/// Get begin.
 	Iterator getBegin()
@@ -265,59 +254,197 @@ public:
 		return m_root == nullptr;
 	}
 
-	/// Copy an element in the map.
-	template<typename TAllocator>
-	void pushBack(TAllocator alloc, const Key& key, ConstReference x)
+	/// Find item.
+	Iterator find(const Key& key);
+
+protected:
+	/// @privatesection
+	TNode* m_root = nullptr;
+
+	void move(HashMapBase& b)
 	{
-		Node* node = alloc.template newInstance<Node>(x);
-		node->m_hash = THasher()(key);
-		pushBackNode(node);
+		m_root = b.m_root;
+		b.m_root = nullptr;
 	}
 
+	/// Add a node in the tree.
+	void insertNode(TNode* node);
+
+	/// Remove a node from the tree.
+	void removeNode(TNode* node);
+};
+
+} // end namespace detail
+
+/// Hash map template.
+template<typename TKey, typename TValue, typename THasher, typename TCompare>
+class HashMap: public detail::HashMapBase<TKey, TValue, THasher, TCompare,
+	detail::HashMapNode<TValue>>
+{
+private:
+	using Base = detail::HashMapBase<TKey, TValue, THasher, TCompare,
+		detail::HashMapNode<TValue>>;
+	using Node = detail::HashMapNode<TValue>;
+
+public:
+	/// Default constructor.
+	HashMap()
+		: Base()
+	{}
+
+	/// Move.
+	HashMap(HashMap&& b)
+		: Base()
+	{
+		Base::move(b);
+	}
+
+	/// You need to manually destroy the map.
+	/// @see HashMap::destroy
+	~HashMap()
+	{
+		ANKI_ASSERT(Base::m_root == nullptr && "Requires manual destruction");
+	}
+
+	/// Move.
+	HashMap& operator=(HashMap&& b)
+	{
+		Base::move(b);
+		return *this;
+	}
+
+	/// Destroy the list.
+	template<typename TAllocator>
+	void destroy(TAllocator alloc);
+
 	/// Copy an element in the map.
 	template<typename TAllocator>
-	void pushBack(Pointer x)
+	void pushBack(TAllocator alloc, const TKey& key, const TValue& x)
 	{
-		static_assert(T_NODE_SAME_AS_T_VALUE == true, "Cannot use that");
-		ANKI_ASSERT(x);
-		ANKI_ASSERT(x->m_hash != 0);
-		pushBackNode(x);
+		Node* node = alloc.template newInstance<Node>(x);
+		node->m_hash = THasher()(key);
+		Base::insertNode(node);
 	}
 
 	/// Construct an element inside the map.
 	template<typename TAllocator, typename... TArgs>
-	void emplaceBack(TAllocator alloc, const Key& key, TArgs&&... args)
+	void emplaceBack(TAllocator alloc, const TKey& key, TArgs&&... args)
 	{
 		Node* node = alloc.template newInstance<Node>(
 			std::forward<TArgs>(args)...);
 		node->m_hash = THasher()(key);
-		pushBackNode(node);
+		Base::insertNode(node);
 	}
 
 	/// Erase element.
 	template<typename TAllocator>
-	void erase(TAllocator alloc, Iterator it);
+	void erase(TAllocator alloc, typename Base::Iterator it)
+	{
+		Node* del = it.m_node;
+		Base::removeNode(del);
+		alloc.deleteInstance(del);
+	}
 
-	/// Find item.
-	Iterator find(const Key& key);
+private:
+	template<typename TAllocator>
+	void destroyInternal(TAllocator alloc, Node* node);
+};
+
+/// The classes that will use the HashMapAllocFree need to inherit from this
+/// one.
+template<typename TClass>
+class HashMapAllocFreeEnabled
+{
+	template<typename TKey, typename TValue, typename THasher,
+		typename TCompare, typename TNode>
+	friend class detail::HashMapBase;
+
+	template<typename TNodePointer, typename TValuePointer,
+		typename TValueReference>
+	friend class detail::HashMapIterator;
+
+	template<typename TKey, typename TValue, typename THasher,
+		typename TCompare>
+	friend class HashMapAllocFree;
+
+	friend TClass;
 
 private:
-	Node* m_root = nullptr;
+	U64 m_hash;
+	TClass* m_left;
+	TClass* m_right;
+	TClass* m_parent; ///< Used for iterating.
+
+	HashMapAllocFreeEnabled()
+		: m_hash(0)
+		, m_left(nullptr)
+		, m_right(nullptr)
+		, m_parent(nullptr)
+	{}
 
-	void move(HashMap& b)
+	TClass& getValue()
 	{
-		ANKI_ASSERT(isEmpty() && "Cannot move before destroying");
-		m_root = b.m_root;
-		b.m_root = nullptr;
+		return *static_cast<TClass*>(this);
 	}
 
-	void pushBackNode(Node* node);
+	const TClass& getValue() const
+	{
+		return *static_cast<const TClass*>(this);
+	}
+};
 
-	template<typename TAllocator>
-	void destroyInternal(TAllocator alloc, Node* node);
+/// Hash map that doesn't do any allocations. To work the TValue nodes will
+/// have to inherit from HashMapAllocFree.
+template<typename TKey, typename TValue, typename THasher, typename TCompare>
+class HashMapAllocFree: public detail::HashMapBase<TKey, TValue, THasher,
+	TCompare, TValue>
+{
+private:
+	using Base = detail::HashMapBase<TKey, TValue, THasher, TCompare, TValue>;
+	using Node = TValue;
+
+public:
+	/// Default constructor.
+	HashMapAllocFree()
+		: Base()
+	{}
+
+	/// Move.
+	HashMapAllocFree(HashMapAllocFree&& b)
+		: Base()
+	{
+		Base::move(b);
+	}
+
+	~HashMapAllocFree() = default;
+
+	/// Move.
+	HashMapAllocFree& operator=(HashMapAllocFree&& b)
+	{
+		Base::move(b);
+		return *this;
+	}
+
+	/// Add an element to the map.
+	void pushBack(const TKey& key, TValue* x)
+	{
+		ANKI_ASSERT(x);
+		HashMapAllocFreeEnabled<TValue>* e =
+			static_cast<HashMapAllocFreeEnabled<TValue>*>(x);
+		e->m_hash = THasher()(key);
+		Base::insertNode(x);
+	}
+
+	/// Erase element.
+	void erase(typename Base::Iterator it)
+	{
+		Node* del = it.m_node;
+		Base::removeNode(del);
+	}
 };
 /// @}
 
 } // end namespace anki
 
 #include "anki/util/HashMap.inl.h"
+

include/anki/util/HashMap.inl.h

@@ -4,46 +4,17 @@
 // http://www.anki3d.org/LICENSE
 
 namespace anki {
+namespace detail {
 
 //==============================================================================
-template<typename TKey, typename TValue, typename THasher, typename TCompare,
-	typename TNode>
-template<typename TAllocator>
-void HashMap<TKey, TValue, THasher, TCompare, TNode>::destroy(TAllocator alloc)
-{
-	if(m_root)
-	{
-		destroyInternal(alloc, m_root);
-		m_root = nullptr;
-	}
-}
-
+// HashMapBase                                                                 =
 //==============================================================================
-template<typename TKey, typename TValue, typename THasher, typename TCompare,
-	typename TNode>
-template<typename TAllocator>
-void HashMap<TKey, TValue, THasher, TCompare, TNode>
-	::destroyInternal(TAllocator alloc, Node* node)
-{
-	ANKI_ASSERT(node);
-
-	if(node->m_right)
-	{
-		destroyInternal(alloc, node->m_right);
-	}
-
-	if(node->m_left)
-	{
-		destroyInternal(alloc, node->m_left);
-	}
-
-	alloc.deleteInstance(node);
-}
 
 //==============================================================================
 template<typename TKey, typename TValue, typename THasher, typename TCompare,
 	typename TNode>
-void HashMap<TKey, TValue, THasher, TCompare, TNode>::pushBackNode(Node* node)
+void HashMapBase<TKey, TValue, THasher, TCompare, TNode>::insertNode(
+	TNode* node)
 {
 	if(ANKI_UNLIKELY(!m_root))
 	{
@@ -52,7 +23,7 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::pushBackNode(Node* node)
 	}
 
 	const U64 hash = node->m_hash;
-	Node* it = m_root;
+	TNode* it = m_root;
 	Bool done = false;
 	do
 	{
@@ -60,7 +31,7 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::pushBackNode(Node* node)
 		if(hash > nhash)
 		{
 			// Go to right
-			Node* right = it->m_right;
+			TNode* right = it->m_right;
 			if(right)
 			{
 				it = right;
@@ -76,7 +47,7 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::pushBackNode(Node* node)
 		{
 			ANKI_ASSERT(hash != nhash && "Not supported");
 			// Go to left
-			Node* left = it->m_left;
+			TNode* left = it->m_left;
 			if(left)
 			{
 				it = left;
@@ -94,13 +65,13 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::pushBackNode(Node* node)
 //==============================================================================
 template<typename TKey, typename TValue, typename THasher, typename TCompare,
 	typename TNode>
-typename HashMap<TKey, TValue, THasher, TCompare, TNode>::Iterator
-	HashMap<TKey, TValue, THasher, TCompare, TNode>
+typename HashMapBase<TKey, TValue, THasher, TCompare, TNode>::Iterator
+	HashMapBase<TKey, TValue, THasher, TCompare, TNode>
 	::find(const Key& key)
 {
 	const U64 hash = THasher()(key);
 
-	Node* node = m_root;
+	TNode* node = m_root;
 	while(node)
 	{
 		const U64 bhash = node->m_hash;
@@ -126,17 +97,12 @@ typename HashMap<TKey, TValue, THasher, TCompare, TNode>::Iterator
 //==============================================================================
 template<typename TKey, typename TValue, typename THasher, typename TCompare,
 	typename TNode>
-template<typename TAllocator>
-void HashMap<TKey, TValue, THasher, TCompare, TNode>::erase(TAllocator alloc,
-	Iterator it)
+void HashMapBase<TKey, TValue, THasher, TCompare, TNode>::removeNode(TNode* del)
 {
-	Node* del = it.m_node;
 	ANKI_ASSERT(del);
-	Node* parent = del->m_parent;
-	Node* left = del->m_left;
-	Node* right = del->m_right;
-
-	alloc.deleteInstance(del);
+	TNode* parent = del->m_parent;
+	TNode* left = del->m_left;
+	TNode* right = del->m_right;
 
 	if(parent)
 	{
@@ -157,14 +123,14 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::erase(TAllocator alloc,
 		{
 			ANKI_ASSERT(left->m_parent == del);
 			left->m_parent = nullptr;
-			pushBackNode(left);
+			insertNode(left);
 		}
 
 		if(right)
 		{
 			ANKI_ASSERT(right->m_parent == del);
 			right->m_parent = nullptr;
-			pushBackNode(right);
+			insertNode(right);
 		}
 	}
 	else
@@ -181,7 +147,7 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::erase(TAllocator alloc,
 			if(right)
 			{
 				right->m_parent = nullptr;
-				pushBackNode(right);
+				insertNode(right);
 			}
 		}
 		else
@@ -196,5 +162,44 @@ void HashMap<TKey, TValue, THasher, TCompare, TNode>::erase(TAllocator alloc,
 	}
 }
 
+} // end namespace detail
+
+//==============================================================================
+// HashMap                                                                     =
+//==============================================================================
+
+//==============================================================================
+template<typename TKey, typename TValue, typename THasher, typename TCompare>
+template<typename TAllocator>
+void HashMap<TKey, TValue, THasher, TCompare>::destroy(TAllocator alloc)
+{
+	if(Base::m_root)
+	{
+		destroyInternal(alloc, Base::m_root);
+		Base::m_root = nullptr;
+	}
+}
+
+//==============================================================================
+template<typename TKey, typename TValue, typename THasher, typename TCompare>
+template<typename TAllocator>
+void HashMap<TKey, TValue, THasher, TCompare>
+	::destroyInternal(TAllocator alloc, Node* node)
+{
+	ANKI_ASSERT(node);
+
+	if(node->m_right)
+	{
+		destroyInternal(alloc, node->m_right);
+	}
+
+	if(node->m_left)
+	{
+		destroyInternal(alloc, node->m_left);
+	}
+
+	alloc.deleteInstance(node);
+}
+
 } // end namespace anki
 

tests/util/HashMap.cpp

@@ -181,21 +181,38 @@ ANKI_TEST(Util, HashMap)
 	}
 }
 
-class Hashable
+class Hashable: public HashMapAllocFreeEnabled<Hashable>
 {
-	ANKI_ENABLE_HASH_MAP(Hashable)
 public:
-	Hashable(U64 hash, int x)
-		: m_hash(hash)
-		, m_x(x)
+	Hashable(int x)
+		: m_x(x)
 	{}
 
 	int m_x;
 };
 
-ANKI_TEST(Util, HashMap_enableHashMap)
+ANKI_TEST(Util, HashMapAllocFree)
 {
-	Hashable a(1, 1);
-	Hashable b(2, 2);
-	Hashable c(10, 10);
+	Hashable a(1);
+	Hashable b(2);
+	Hashable c(10);
+
+	HashMapAllocFree<int, Hashable, Hasher, Compare> map;
+
+	// Add vals
+	map.pushBack(1, &a);
+	map.pushBack(2, &b);
+	map.pushBack(10, &c);
+
+	// Find
+	ANKI_TEST_EXPECT_EQ(map.find(1)->m_x, 1);
+	ANKI_TEST_EXPECT_EQ(map.find(10)->m_x, 10);
+
+	// Erase
+	map.erase(map.find(10));
+	ANKI_TEST_EXPECT_EQ(map.find(10), map.getEnd());
+
+	// Put bach
+	map.pushBack(10, &c);
+	ANKI_TEST_EXPECT_NEQ(map.find(10), map.getEnd());
 }