Skip to content

Commit

Permalink
marl::containers::vector fixes
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ben-clayton committed Jun 4, 2020
1 parent fcbe1f2 commit c277c61
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 0 deletions.
17 changes: 17 additions & 0 deletions include/marl/containers.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>&);

Expand All @@ -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;
Expand Down Expand Up @@ -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=(
Expand Down Expand Up @@ -238,6 +254,7 @@ void vector<T, BASE_CAPACITY>::free() {

if (allocation.ptr != nullptr) {
allocator->free(allocation);
allocation = {};
elements = nullptr;
}
}
Expand Down
47 changes: 47 additions & 0 deletions src/containers_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,60 @@ 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");
ASSERT_EQ(vectorB[1], "B");
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);

Expand Down

0 comments on commit c277c61

Please sign in to comment.