Address PR name consistency comments, adds copy contructor logic for DoublyLinkedList so that lists of trivially copyable types get copied more efficiently

Author: Fletterio

include/nbl/core/containers/DoublyLinkedList.h

@@ -74,13 +74,18 @@ class DoublyLinkedList
 	template <bool Mutable>
 	friend class Iterator;
 
+	using iterator = Iterator<true>;
+	using const_iterator = Iterator<false>;
+
 	using allocator_t = allocator;
 	using allocator_traits_t = std::allocator_traits<allocator_t>;
 	using address_allocator_t = PoolAddressAllocator<uint32_t>;
 	using node_t = SDoublyLinkedNode<Value>;
 	using value_t = Value;
 	using disposal_func_t = std::function<void(value_t&)>;
 
+	static constexpr bool IsTriviallyCopyable = std::is_trivially_copyable_v<Value>;
+
 	_NBL_STATIC_INLINE_CONSTEXPR uint32_t invalid_iterator = node_t::invalid_iterator;
 
 	// get the fixed capacity
@@ -238,23 +243,42 @@ class DoublyLinkedList
 		// Offset the array start by the storage used by the address allocator
 		m_array = reinterpret_cast<node_t*>(reinterpret_cast<uint8_t*>(m_reservedSpace) + addressAllocatorStorageSize * sizeof(node_t));
 
-		m_addressAllocator = address_allocator_t(m_reservedSpace, 0u, 0u, 1u, capacity, 1u);
 		// If allocation failed, create list with no capacity to indicate creation failed
 		m_cap = m_reservedSpace ? capacity : 0;
-		m_back = invalid_iterator;
-		m_begin = invalid_iterator;
+		m_addressAllocator = address_allocator_t(m_reservedSpace, 0u, 0u, 1u, m_cap, 1u);
 	}
 
 	DoublyLinkedList() = default;
 
-	// Unless there's a way of knowing whether for our type a shallow copy is the same as invoking the copy constructor,
-	// we must iterate over the whole list and copy instance by instance 
-	explicit DoublyLinkedList(const DoublyLinkedList& other) : DoublyLinkedList(other.m_cap, disposal_func_t(other.m_dispose_f), other.m_allocator)
+	// Copy Constructor
+	explicit DoublyLinkedList(const DoublyLinkedList& other) : m_dispose_f(other.m_dispose_f), m_allocator(other.m_allocator)
 	{
+		const size_t addressAllocatorStorageSize = (address_allocator_t::reserved_size(1u, other.m_cap, 1u) + sizeof(node_t) - 1) / sizeof(node_t);
+		m_currentAllocationSize = addressAllocatorStorageSize + other.m_cap;
+		m_reservedSpace = reinterpret_cast<void*>(allocator_traits_t::allocate(m_allocator, m_currentAllocationSize));
+		// If allocation failed, create a list with no capacity
+		m_cap = m_reservedSpace ? other.m_cap : 0;
 		if (!m_cap) return; // Allocation failed
-		// Reverse iteration since we push from the front
-		for (auto it = other.crbegin(); it != other.crend(); it++)
-			pushFront(value_t(*it));
+		// Offset the array start by the storage used by the address allocator
+		m_array = reinterpret_cast<node_t*>(reinterpret_cast<uint8_t*>(m_reservedSpace) + addressAllocatorStorageSize * sizeof(node_t));
+
+		if constexpr (IsTriviallyCopyable)
+		{
+			// Create new address allocator by copying state
+			m_addressAllocator = address_allocator_t(m_cap, other.m_addressAllocator, m_reservedSpace);
+			// Copy memory over
+			memcpy(m_array, other.m_array, m_cap * sizeof(node_t));
+			m_back = other.m_back;
+			m_begin = other.m_begin;
+		}
+		else
+		{
+			m_addressAllocator = address_allocator_t(m_reservedSpace, 0u, 0u, 1u, m_cap, 1u);
+			// Reverse iteration since we push from the front
+			for (auto it = other.crbegin(); it != other.crend(); it++)
+				pushFront(value_t(*it));
+
+		}
 	}
 
 	DoublyLinkedList& operator=(const DoublyLinkedList& other) = delete;
@@ -287,14 +311,14 @@ class DoublyLinkedList
 	}
 
 	// Iterator stuff
-	Iterator<true> begin();
-	Iterator<true> end();
-	Iterator<false> cbegin() const;
-	Iterator<false> cend() const;
-	std::reverse_iterator<Iterator<true>> rbegin();
-	std::reverse_iterator<Iterator<true>> rend();
-	std::reverse_iterator<Iterator<false>> crbegin() const;
-	std::reverse_iterator<Iterator<false>> crend() const;
+	iterator begin();
+	iterator end();
+	const_iterator cbegin() const;
+	const_iterator cend() const;
+	std::reverse_iterator<iterator> rbegin();
+	std::reverse_iterator<iterator> rend();
+	std::reverse_iterator<const_iterator> crbegin() const;
+	std::reverse_iterator<const_iterator> crend() const;
 
 private:
 	//allocate and get the address of the next free node
@@ -362,8 +386,8 @@ class DoublyLinkedList
 	node_t* m_array;
 
 	uint32_t m_cap;
-	uint32_t m_back;
-	uint32_t m_begin;
+	uint32_t m_back = invalid_iterator;
+	uint32_t m_begin = invalid_iterator;
 	disposal_func_t m_dispose_f;
 };
 
@@ -437,51 +461,51 @@ class DoublyLinkedList<Value, allocator>::Iterator
 };
 
 template<typename Value, class allocator>
-DoublyLinkedList<Value, allocator>::Iterator<true> DoublyLinkedList<Value, allocator>::begin()
+DoublyLinkedList<Value, allocator>::iterator DoublyLinkedList<Value, allocator>::begin()
 {
-	return Iterator<true>(this, m_begin);
+	return iterator(this, m_begin);
 }
 
 template<typename Value, class allocator>
-DoublyLinkedList<Value, allocator>::Iterator<false> DoublyLinkedList<Value, allocator>::cbegin() const
+DoublyLinkedList<Value, allocator>::const_iterator DoublyLinkedList<Value, allocator>::cbegin() const
 {
-	return Iterator<false>(this, m_begin);
+	return const_iterator(this, m_begin);
 }
 
 template<typename Value, class allocator>
-DoublyLinkedList<Value, allocator>::Iterator<true> DoublyLinkedList<Value, allocator>::end()
+DoublyLinkedList<Value, allocator>::iterator DoublyLinkedList<Value, allocator>::end()
 {
-	return Iterator<true>(this, invalid_iterator);
+	return iterator(this, invalid_iterator);
 }
 
 template<typename Value, class allocator>
-DoublyLinkedList<Value, allocator>::Iterator<false> DoublyLinkedList<Value, allocator>::cend() const
+DoublyLinkedList<Value, allocator>::const_iterator DoublyLinkedList<Value, allocator>::cend() const
 {
-	return Iterator<false>(this, invalid_iterator);
+	return const_iterator(this, invalid_iterator);
 }
 
 template<typename Value, class allocator>
-std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::Iterator<true>> DoublyLinkedList<Value, allocator>::rbegin()
+std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::iterator> DoublyLinkedList<Value, allocator>::rbegin()
 {
-	return std::reverse_iterator<Iterator<true>>(Iterator<true>(this, invalid_iterator));
+	return std::reverse_iterator<iterator>(iterator(this, invalid_iterator));
 }
 
 template<typename Value, class allocator>
-std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::Iterator<false>> DoublyLinkedList<Value, allocator>::crbegin() const
+std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::const_iterator> DoublyLinkedList<Value, allocator>::crbegin() const
 {
-	return std::reverse_iterator<Iterator<false>>(Iterator<false>(this, invalid_iterator));
+	return std::reverse_iterator<const_iterator>(const_iterator(this, invalid_iterator));
 }
 
 template<typename Value, class allocator>
-std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::Iterator<true>> DoublyLinkedList<Value, allocator>::rend()
+std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::iterator> DoublyLinkedList<Value, allocator>::rend()
 {
-	return std::reverse_iterator<Iterator<true>>(Iterator<true>(this, m_begin));
+	return std::reverse_iterator<iterator>(iterator(this, m_begin));
 }
 
 template<typename Value, class allocator>
-std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::Iterator<false>> DoublyLinkedList<Value, allocator>::crend() const
+std::reverse_iterator<typename DoublyLinkedList<Value, allocator>::const_iterator> DoublyLinkedList<Value, allocator>::crend() const
 {
-	return std::reverse_iterator<Iterator<false>>(Iterator<false>(this, m_begin));
+	return std::reverse_iterator<const_iterator>(const_iterator(this, m_begin));
 }
 
 } //namespace core

include/nbl/core/containers/LRUCache.h

@@ -38,11 +38,6 @@ class LRUCacheBase
 		LRUCacheBase(const uint32_t capacity, MapHash&& _hash, MapEquals&& _equals, disposal_func_t&& df) : m_list(capacity, std::move(df)), m_hash(std::move(_hash)), m_equals(std::move(_equals)), searchedKey(nullptr)
 		{ }
 
-		// CAREFUL: searchedKey was left as nullptr because for ResizableLRUCache, it's always set by some method before being used
-		// If you implement another LRUCache where this is not the case and the state of the copy has to match exactly, it needs to be consistent.
-		// What I would do in such a case (either in this same class or dividing this class again into two, one that leaves the searchedKey null and 
-		// another that sets it to a consistent value) would be to iterate over every Key-Value pair in the copy, and set searchedKey to point
-		// to the Key such that `MapEquals(Key, *other.searchedKey)`
 		LRUCacheBase(const LRUCacheBase& other) : m_list(other.m_list), m_hash(other.m_hash), m_equals(other.m_equals), searchedKey(nullptr) {}
 
 	public:
@@ -298,8 +293,8 @@ class ResizableLRUCache : protected impl::LRUCacheBase<Key, Value, MapHash, MapE
 
 public:
 	// Keep it simple
-	template <bool Mutable>
-	using Iterator = typename list_t::template Iterator<Mutable>;
+	using iterator = typename list_t::iterator;
+	using const_iterator = typename list_t::const_iterator;
 
 	using disposal_func_t = typename base_t::disposal_func_t;
 	using assoc_t = typename base_t::list_value_t;
@@ -313,7 +308,6 @@ class ResizableLRUCache : protected impl::LRUCacheBase<Key, Value, MapHash, MapE
 	}
 	ResizableLRUCache() = delete;
 
-	// COPY CONST
 	// It's not possible to copy the unordered_set memory-wise and just change hashing and KeyEquals functions unfortunately
 	// (in the general case that wouldn't make sense but it does here due to the way the wrappers work)
 	// Anyway, we must iterate over the old cache and copy the map over
@@ -485,14 +479,14 @@ class ResizableLRUCache : protected impl::LRUCacheBase<Key, Value, MapHash, MapE
 
 	// Iterator stuff
 	// Normal iterator order is MRU -> LRU
-	Iterator<true> begin() { return base_t::m_list.begin(); }
-	Iterator<true> end() { return base_t::m_list.end(); }
-	Iterator<false> cbegin() const { return base_t::m_list.cbegin(); }
-	Iterator<false> cend() const { return base_t::m_list.cend(); }
-	std::reverse_iterator<Iterator<true>> rbegin() { return base_t::m_list.rbegin(); }
-	std::reverse_iterator<Iterator<true>> rend() { return base_t::m_list.rend(); }
-	std::reverse_iterator<Iterator<false>> crbegin() const { return base_t::m_list.crbegin(); }
-	std::reverse_iterator<Iterator<false>> crend() const { return base_t::m_list.crend(); }
+	iterator begin() { return base_t::m_list.begin(); }
+	iterator end() { return base_t::m_list.end(); }
+	const_iterator cbegin() const { return base_t::m_list.cbegin(); }
+	const_iterator cend() const { return base_t::m_list.cend(); }
+	std::reverse_iterator<iterator> rbegin() { return base_t::m_list.rbegin(); }
+	std::reverse_iterator<iterator> rend() { return base_t::m_list.rend(); }
+	std::reverse_iterator<const_iterator> crbegin() const { return base_t::m_list.crbegin(); }
+	std::reverse_iterator<const_iterator> crend() const { return base_t::m_list.crend(); }
 
 protected:
 	unordered_set<uint32_t, WrapHash, WrapEquals> m_shortcut_map;