From 96b3c1d2a37c0bffa0d4ffd0ebbb5d82786c6eaa Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Thu, 4 Jun 2020 11:25:23 +0100
Subject: [PATCH] marl::containers::vector fixes

Default copy constructor and copy assignment operators were still being generated, which produce bad implementations due to raw internal allocations.

The tests were only exercising copy and assignment for vectors of different base capacities, which we do have non-default implementations for.

Delete the copy constructor - it discourages use allocators, which is a Bad Thing.

Implement the assignment operator.

Add tests.

Note: Nothing in marl was exercising these broken default constructors.
---
 include/marl/containers.h | 17 ++++++++++++++
 src/containers_test.cpp   | 47 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/include/marl/containers.h b/include/marl/containers.h
index acf421c..337ab3d 100644
--- a/include/marl/containers.h
+++ b/include/marl/containers.h
@@ -49,6 +49,8 @@ class vector {
 
   inline ~vector();
 
+  inline vector& operator=(const vector&);
+
   template <int BASE_CAPACITY_2>
   inline vector<T, BASE_CAPACITY>& operator=(const vector<T, BASE_CAPACITY_2>&);
 
@@ -72,6 +74,8 @@ class vector {
  private:
   using TStorage = typename marl::aligned_storage<sizeof(T), alignof(T)>::type;
 
+  vector(const vector&) = delete;
+
   inline void free();
 
   Allocator* const allocator;
@@ -110,6 +114,18 @@ vector<T, BASE_CAPACITY>::~vector() {
   free();
 }
 
+template <typename T, int BASE_CAPACITY>
+vector<T, BASE_CAPACITY>& vector<T, BASE_CAPACITY>::operator=(
+    const vector& other) {
+  free();
+  reserve(other.size());
+  count = other.size();
+  for (size_t i = 0; i < count; i++) {
+    new (&reinterpret_cast<T*>(elements)[i]) T(other[i]);
+  }
+  return *this;
+}
+
 template <typename T, int BASE_CAPACITY>
 template <int BASE_CAPACITY_2>
 vector<T, BASE_CAPACITY>& vector<T, BASE_CAPACITY>::operator=(
@@ -238,6 +254,7 @@ void vector<T, BASE_CAPACITY>::free() {
 
   if (allocation.ptr != nullptr) {
     allocator->free(allocation);
+    allocation = {};
     elements = nullptr;
   }
 }
diff --git a/src/containers_test.cpp b/src/containers_test.cpp
index 2bc981c..77723af 100644
--- a/src/containers_test.cpp
+++ b/src/containers_test.cpp
@@ -132,6 +132,21 @@ TEST_F(ContainersVectorTest, CopyConstruct) {
   vectorA[1] = "B";
   vectorA[2] = "C";
 
+  marl::containers::vector<std::string, 4> vectorB(vectorA, allocator);
+  ASSERT_EQ(vectorB.size(), size_t(3));
+  ASSERT_EQ(vectorB[0], "A");
+  ASSERT_EQ(vectorB[1], "B");
+  ASSERT_EQ(vectorB[2], "C");
+}
+
+TEST_F(ContainersVectorTest, CopyConstructDifferentBaseCapacity) {
+  marl::containers::vector<std::string, 4> vectorA(allocator);
+
+  vectorA.resize(3);
+  vectorA[0] = "A";
+  vectorA[1] = "B";
+  vectorA[2] = "C";
+
   marl::containers::vector<std::string, 2> vectorB(vectorA, allocator);
   ASSERT_EQ(vectorB.size(), size_t(3));
   ASSERT_EQ(vectorB[0], "A");
@@ -139,6 +154,38 @@ TEST_F(ContainersVectorTest, CopyConstruct) {
   ASSERT_EQ(vectorB[2], "C");
 }
 
+TEST_F(ContainersVectorTest, CopyAssignment) {
+  marl::containers::vector<std::string, 4> vectorA(allocator);
+
+  vectorA.resize(3);
+  vectorA[0] = "A";
+  vectorA[1] = "B";
+  vectorA[2] = "C";
+
+  marl::containers::vector<std::string, 4> vectorB(allocator);
+  vectorB = vectorA;
+  ASSERT_EQ(vectorB.size(), size_t(3));
+  ASSERT_EQ(vectorB[0], "A");
+  ASSERT_EQ(vectorB[1], "B");
+  ASSERT_EQ(vectorB[2], "C");
+}
+
+TEST_F(ContainersVectorTest, CopyAssignmentDifferentBaseCapacity) {
+  marl::containers::vector<std::string, 4> vectorA(allocator);
+
+  vectorA.resize(3);
+  vectorA[0] = "A";
+  vectorA[1] = "B";
+  vectorA[2] = "C";
+
+  marl::containers::vector<std::string, 2> vectorB(allocator);
+  vectorB = vectorA;
+  ASSERT_EQ(vectorB.size(), size_t(3));
+  ASSERT_EQ(vectorB[0], "A");
+  ASSERT_EQ(vectorB[1], "B");
+  ASSERT_EQ(vectorB[2], "C");
+}
+
 TEST_F(ContainersVectorTest, MoveConstruct) {
   marl::containers::vector<std::string, 4> vectorA(allocator);