Browse Source

Made List work with movable non-copyable types.

David Piuva 7 months ago
parent
commit
2ed3b5d697
2 changed files with 46 additions and 17 deletions
  1. 11 16
      Source/DFPSR/collection/List.h
  2. 35 1
      Source/test/tests/ListTest.cpp

+ 11 - 16
Source/DFPSR/collection/List.h

@@ -46,6 +46,7 @@ private:
 	T *impl_elements = nullptr;
 	T *impl_elements = nullptr;
 	intptr_t impl_length = 0;
 	intptr_t impl_length = 0;
 	intptr_t impl_buffer_length = 0;
 	intptr_t impl_buffer_length = 0;
+
 	// Makes sure that there is memory available for storing at least minimumAllocatedLength elements.
 	// Makes sure that there is memory available for storing at least minimumAllocatedLength elements.
 	void impl_reserve(intptr_t minimumAllocatedLength) {
 	void impl_reserve(intptr_t minimumAllocatedLength) {
 		if (minimumAllocatedLength > this->impl_buffer_length) {
 		if (minimumAllocatedLength > this->impl_buffer_length) {
@@ -60,17 +61,9 @@ private:
 			uintptr_t availableSize = heap_getAllocationSize(newAllocation.header);
 			uintptr_t availableSize = heap_getAllocationSize(newAllocation.header);
 			heap_setUsedSize(newAllocation.header, availableSize);
 			heap_setUsedSize(newAllocation.header, availableSize);
 			// Move the data from the old allocation to the new allocation.
 			// Move the data from the old allocation to the new allocation.
-			if (std::is_move_constructible<T>::value) {
-				// If T is move constructible, we do not have to clone the elements.
-				for (intptr_t e = 0; e < this->impl_buffer_length; e++) {
-					new (newElements + e) T(std::move(this->impl_elements[e]));
-				}
-			} else {
-				// If T is not move constructible, we have to create a copy and then destroy the original.
-				for (intptr_t e = 0; e < this->impl_buffer_length; e++) {
-					new (newElements + e) T(this->impl_elements[e]);
-					this->impl_elements[e].~T();
-				}
+			//   The compiler should automatically call a copy constructor if the move operator is deleted.
+			for (intptr_t e = 0; e < this->impl_buffer_length; e++) {
+				new (newElements + e) T(std::move(this->impl_elements[e]));
 			}
 			}
 			// Transfer ownership to the new allocation.
 			// Transfer ownership to the new allocation.
 			heap_decreaseUseCount(this->impl_elements);
 			heap_decreaseUseCount(this->impl_elements);
@@ -94,10 +87,12 @@ private:
 		intptr_t elementCount = sizeof...(args);
 		intptr_t elementCount = sizeof...(args);
 		this->impl_reserve(elementCount);
 		this->impl_reserve(elementCount);
 		this->impl_length = elementCount;
 		this->impl_length = elementCount;
-		std::initializer_list<T> otherArguments = { T(args)... };
-		for (intptr_t e = 0; e < elementCount; e++) {
-			new (this->impl_elements + e) T(otherArguments.begin()[e]);
-		}
+		intptr_t e = 0;
+		(void)std::initializer_list<int>{
+			(
+				new (this->impl_elements + e) T(std::move(args)), e++, 0
+			)...
+		};
 	}
 	}
 public:
 public:
 	// Copy constructor
 	// Copy constructor
@@ -153,7 +148,7 @@ public:
 	  typename... OTHERS,
 	  typename... OTHERS,
 	  DSR_ENABLE_IF(DSR_CONVERTIBLE_TO(FIRST, T))
 	  DSR_ENABLE_IF(DSR_CONVERTIBLE_TO(FIRST, T))
 	>
 	>
-	List(FIRST first, OTHERS... others) {
+	List(FIRST first, OTHERS&&... others) {
 		this->impl_setElements(first, others...);
 		this->impl_setElements(first, others...);
 	}
 	}
 	~List() {
 	~List() {

+ 35 - 1
Source/test/tests/ListTest.cpp

@@ -6,6 +6,21 @@ static void targetByReference(List<int32_t> &target, int32_t value) {
 	target.push(value);
 	target.push(value);
 }
 }
 
 
+struct Unique {
+	String name;
+	// Constructible
+	Unique(const ReadableString &name) : name(name) {}
+	// Movable
+	Unique(Unique &&original) = default;
+	Unique& operator=(Unique &&original) = default;
+	// Destructible
+	~Unique() = default;
+	// Not clonable
+	Unique(const Unique& original) = delete;
+	Unique& operator=(const Unique& original) = delete;
+};
+static_assert(std::is_move_constructible<Unique>::value, "The Unique type should be move constructible!");
+
 START_TEST(List)
 START_TEST(List)
 	{
 	{
 		// Populate
 		// Populate
@@ -129,5 +144,24 @@ START_TEST(List)
 		myOtherStrings.clear();
 		myOtherStrings.clear();
 		ASSERT_EQUAL(myOtherStrings.length(), 0);
 		ASSERT_EQUAL(myOtherStrings.length(), 0);
 	}
 	}
-	// TODO: Test lists of objects that can not be cloned.
+	{
+		// Non-copyable types ensure that the constructed object is not accidentally copied into another location.
+		List<Unique> objects = List<Unique>(Unique(U"One"), Unique(U"Two"));
+		ASSERT_EQUAL(objects.length(), 2);
+		ASSERT_EQUAL(objects[0].name, U"One");
+		ASSERT_EQUAL(objects[1].name, U"Two");
+		// The push method can not be called with on non-copyable element types, because push must copy the given element.
+		//objects.push(Unique(U"Three"));
+		// The pushConstruct method can be used instead to construct the element inside of the list.
+		objects.pushConstruct(U"Three");
+		ASSERT_EQUAL(objects.length(), 3);
+		ASSERT_EQUAL(objects[0].name, U"One");
+		ASSERT_EQUAL(objects[1].name, U"Two");
+		ASSERT_EQUAL(objects[2].name, U"Three");
+		objects.swap(0, 1);
+		ASSERT_EQUAL(objects.length(), 3);
+		ASSERT_EQUAL(objects[0].name, U"Two");
+		ASSERT_EQUAL(objects[1].name, U"One");
+		ASSERT_EQUAL(objects[2].name, U"Three");
+	}
 END_TEST
 END_TEST