From c277c61b036858053cf5eba204d51b18b803b545 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);