From 9e03bc57de46257aa0bd1ce8f2db45a20e59fc01 Mon Sep 17 00:00:00 2001
From: Baptiste Wicht <baptiste.wicht@gmail.com>
Date: Mon, 3 Oct 2016 19:16:21 +0200
Subject: [PATCH] Review vector

Several objects were incorrectly allocated and destructed
---
 tstl/include/deque.hpp  |  90 +++++++++---------------
 tstl/include/vector.hpp | 148 ++++++++++++++++++++++++----------------
 2 files changed, 121 insertions(+), 117 deletions(-)

diff --git a/tstl/include/deque.hpp b/tstl/include/deque.hpp
index 63bd2523..65046f5f 100644
--- a/tstl/include/deque.hpp
+++ b/tstl/include/deque.hpp
@@ -188,12 +188,22 @@ struct deque {
         return *this;
     }
 
-    ~deque() {
+    void destruct_all(){
         for (size_t i = 0; i < _size; ++i) {
             (*this)[i].~value_type();
         }
     }
 
+    ~deque() {
+        destruct_all();
+
+        for(size_t i = 0; i < blocks; ++i){
+            deallocate(data[i]);
+        }
+
+        delete[] data;
+    }
+
     void push_back(T&& element) {
         ensure_capacity_back(1);
 
@@ -201,14 +211,14 @@ struct deque {
             auto block = (last_element + 1) / block_elements;
             auto index = (last_element + 1) % block_elements;
 
-            data[block][index] = std::move(element);
+            new (&data[block][index]) T(std::move(element));
 
             ++last_element;
         } else {
             auto block = last_element / block_elements;
             auto index = last_element % block_elements;
 
-            data[block][index] = std::move(element);
+            new (&data[block][index]) T(std::move(element));
         }
 
         ++_size;
@@ -221,41 +231,19 @@ struct deque {
             auto block = (last_element + 1) / block_elements;
             auto index = (last_element + 1) % block_elements;
 
-            data[block][index] = element;
+            new (&data[block][index]) T(element);
 
             ++last_element;
         } else {
             auto block = last_element / block_elements;
             auto index = last_element % block_elements;
 
-            data[block][index] = element;
+            new (&data[block][index]) T(element);
         }
 
         ++_size;
     }
 
-    reference_type emplace_back() {
-        ensure_capacity_back(1);
-
-        if (_size) {
-            auto block = (last_element + 1) / block_elements;
-            auto index = (last_element + 1) % block_elements;
-
-            new (&data[block][index]) T();
-
-            ++last_element;
-        } else {
-            auto block = last_element / block_elements;
-            auto index = last_element % block_elements;
-
-            new (&data[block][index]) T();
-        }
-
-        ++_size;
-
-        return back();
-    }
-
     template <typename... Args>
     reference_type emplace_back(Args&&... args) {
         ensure_capacity_back(1);
@@ -286,14 +274,14 @@ struct deque {
             auto block = (first_element - 1) / block_elements;
             auto index = (first_element - 1) % block_elements;
 
-            data[block][index] = std::move(element);
+            new (&data[block][index]) T(std::move(element));
 
             --first_element;
         } else {
             auto block = first_element / block_elements;
             auto index = first_element % block_elements;
 
-            data[block][index] = std::move(element);
+            new (&data[block][index]) T(std::move(element));
         }
 
         ++_size;
@@ -306,34 +294,14 @@ struct deque {
             auto block = (first_element - 1) / block_elements;
             auto index = (first_element - 1) % block_elements;
 
-            data[block][index] = element;
+            new (&data[block][index]) T(element);
 
             --first_element;
         } else {
             auto block = first_element / block_elements;
             auto index = first_element % block_elements;
 
-            data[block][index] = element;
-        }
-
-        ++_size;
-    }
-
-    reference_type emplace_front() {
-        ensure_capacity_front(1);
-
-        if (_size) {
-            auto block = (first_element - 1) / block_elements;
-            auto index = (first_element - 1) % block_elements;
-
-            new (&data[block][index]) T();
-
-            --first_element;
-        } else {
-            auto block = first_element / block_elements;
-            auto index = first_element % block_elements;
-
-            new (&data[block][index]) T();
+            new (&data[block][index]) T(element);
         }
 
         ++_size;
@@ -424,9 +392,7 @@ struct deque {
      * \brief Removes all the elements of the deque
      */
     void clear() {
-        for (size_t i = 0; i < _size; ++i) {
-            (*this)[i].~value_type();
-        }
+        destruct_all();
 
         first_element = 0;
         last_element  = 0;
@@ -509,6 +475,14 @@ struct deque {
     }
 
 private:
+    static value_type* allocate(size_t n){
+        return static_cast<value_type*>(malloc(n * sizeof(value_type)));
+    }
+
+    static void deallocate(value_type* ptr){
+        free(ptr);
+    }
+
     void ensure_capacity_front(size_t n) {
         auto capacity_front = first_element;
 
@@ -519,7 +493,7 @@ private:
                 data = new T*[blocks];
 
                 for (size_t i = 0; i < blocks; ++i) {
-                    data[i] = new T[block_elements];
+                    data[i] = allocate(block_elements);
                 }
 
                 first_element = blocks * block_elements - 1;
@@ -535,7 +509,7 @@ private:
                 }
 
                 for (size_t i = 0; i < new_blocks; ++i) {
-                    new_data[i] = new T[block_elements];
+                    new_data[i] = allocate(block_elements);
                 }
 
                 first_element += new_blocks * block_elements;
@@ -559,7 +533,7 @@ private:
                 data = new T*[blocks];
 
                 for (size_t i = 0; i < blocks; ++i) {
-                    data[i] = new T[block_elements];
+                    data[i] = allocate(block_elements);
                 }
 
                 first_element = 0;
@@ -575,7 +549,7 @@ private:
                 }
 
                 for (size_t i = blocks; i < blocks + new_blocks; ++i) {
-                    new_data[i] = new T[block_elements];
+                    new_data[i] = allocate(block_elements);
                 }
 
                 delete[] data;
diff --git a/tstl/include/vector.hpp b/tstl/include/vector.hpp
index d8d683a5..e9ef6db4 100644
--- a/tstl/include/vector.hpp
+++ b/tstl/include/vector.hpp
@@ -40,40 +40,41 @@ struct vector {
     /*!
      * \brief Constructs a vector of the given size
      */
-    explicit vector(uint64_t c) : data(new T[c]), _size(0), _capacity(c) {}
+    explicit vector(uint64_t c) : data(allocate(c)), _size(0), _capacity(c) {}
 
     /*!
      * \brief Construct a vector containing the given values
      */
-    vector(initializer_list<T> values) : data(new T[values.size()]), _size(values.size()), _capacity(values.size()) {
+    vector(initializer_list<T> values) : data(allocate(values.size())), _size(values.size()), _capacity(values.size()) {
         std::copy(values.begin(), values.end(), begin());
     }
 
     vector(const vector& rhs) : data(nullptr), _size(rhs._size), _capacity(rhs._capacity) {
         if(!rhs.empty()){
-            data = new T[_capacity];
+            data = allocate(_capacity);
 
             for(size_t i = 0; i < _size; ++i){
-                data[i] = rhs.data[i];
+                new (&data[i]) value_type(rhs.data[i]);
             }
         }
     }
 
     vector& operator=(const vector& rhs){
-        if(data && _capacity < rhs._capacity){
-            delete[] data;
-            data = nullptr;
-        }
-
         if(_capacity < rhs._capacity){
+            if(data){
+                release();
+            }
+
             _capacity = rhs._capacity;
-            data = new T[_capacity];
+            data = allocate(_capacity);
+        } else {
+            destruct_all();
         }
 
         _size = rhs._size;
 
         for(size_t i = 0; i < _size; ++i){
-            data[i] = rhs.data[i];
+            new (&data[i]) value_type(rhs.data[i]);
         }
 
         return *this;
@@ -89,7 +90,7 @@ struct vector {
 
     vector& operator=(vector&& rhs){
         if(data){
-            delete[] data;
+            release();
         }
 
         data = rhs.data;
@@ -104,7 +105,7 @@ struct vector {
 
     ~vector(){
         if(data){
-            delete[] data;
+            release();
         }
     }
 
@@ -191,8 +192,11 @@ struct vector {
         if(new_size > size()){
             ensure_capacity(new_size);
 
-            //The elements will automatically be created to their defaults when
-            //the array gets resized in ensure_capacity
+            // Default initialize the new elements
+            for(size_t i = _size; i < new_size; ++i){
+                new (&data[i]) value_type();
+            }
+
             _size = new_size;
         } else if(new_size < _size){
             // Call the necessary destructors
@@ -211,7 +215,7 @@ struct vector {
     void push_back(value_type&& element){
         ensure_capacity(_size + 1);
 
-        data[_size++] = std::move(element);
+        new (&data[_size++]) value_type(std::move(element));
     }
 
     /*!
@@ -220,43 +224,7 @@ struct vector {
     void push_back(const value_type& element){
         ensure_capacity(_size + 1);
 
-        data[_size++] = element;
-    }
-
-    /*!
-     * \brief Add an element at the front of the vector
-     */
-    void push_front(value_type&& element){
-        ensure_capacity(_size + 1);
-
-        for(size_t i = _size; i > 0; --i){
-            data[i] = std::move(data[i-1]);
-        }
-
-        data[0] = std::move(element);
-        ++_size;
-    }
-
-    /*!
-     * \brief Add an element at the front of the vector
-     */
-    void push_front(const value_type& element){
-        ensure_capacity(_size + 1);
-
-        for(size_t i = _size; i > 0; --i){
-            data[i] = std::move(data[i-1]);
-        }
-
-        data[0] = element;
-        ++_size;
-    }
-
-    value_type& emplace_back(){
-        ensure_capacity(_size + 1);
-
-        new (&data[_size++]) T();
-
-        return back();
+        new (&data[_size++]) value_type(element);
     }
 
     template<typename... Args>
@@ -268,6 +236,46 @@ struct vector {
         return back();
     }
 
+    /*!
+     * \brief Add an element at the front of the vector
+     */
+    void push_front(value_type&& element){
+        ensure_capacity(_size + 1);
+
+        if(!empty()){
+            new (&data[_size]) value_type(std::move(data[_size - 1]));
+
+            for (size_t i = _size - 1; i > 0; --i) {
+                data[i] = std::move(data[i - 1]);
+            }
+        }
+
+        // At this point data[0] has been deleted
+        data[0] = std::move(element);
+
+        ++_size;
+    }
+
+    /*!
+     * \brief Add an element at the front of the vector
+     */
+    void push_front(const value_type& element){
+        ensure_capacity(_size + 1);
+
+        if(!empty()){
+            new (&data[_size]) value_type(std::move(data[_size - 1]));
+
+            for (size_t i = _size - 1; i > 0; --i) {
+                data[i] = std::move(data[i - 1]);
+            }
+        }
+
+        // At this point data[0] has been deleted
+        data[0] = element;
+
+        ++_size;
+    }
+
     /*!
      * \brief Removes the last element of the vector
      */
@@ -282,9 +290,7 @@ struct vector {
      * \brief Removes all the elements of the vector
      */
     void clear(){
-        for(size_t i = 0; i < _size; ++i){
-            data[i].~value_type();
-        }
+        destruct_all();
 
         _size = 0;
     }
@@ -383,10 +389,33 @@ struct vector {
     }
 
 private:
+    static value_type* allocate(size_t n){
+        return static_cast<value_type*>(malloc(n * sizeof(value_type)));
+    }
+
+    static void deallocate(value_type* ptr){
+        free(ptr);
+    }
+
+    void destruct_all(){
+        // Call the destructors
+        for(size_t i = 0; i< _size; ++i){
+            data[i].~value_type();
+        }
+    }
+
+    void release(){
+        destruct_all();
+
+        // Deallocate the memory
+        free(data);
+        data = nullptr;
+    }
+
     void ensure_capacity(size_t new_capacity){
         if(_capacity == 0){
             _capacity = new_capacity;
-            data = new T[_capacity];
+            data = allocate(_capacity);
         } else if(_capacity < new_capacity){
             // Double the current capacity
             _capacity= _capacity * 2;
@@ -396,10 +425,11 @@ private:
                 _capacity = new_capacity;
             }
 
-            auto new_data = new T[_capacity];
+            auto new_data = allocate(_capacity);
             std::move_n(data, _size, new_data);
 
-            delete[] data;
+            release();
+
             data = new_data;
         }
     }