Simple re-implementation of std::vector

Multi tool use
Here is a simple reimplementation of most of std::vector
. I want to check if I implement it correctly but the standard libraries source code is not very readable for me.
namespace nonstd {
template<typename Ty>
class vector
{
public:
using iterator = Ty * ;
using const_iterator = const Ty*;
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
private:
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
};
template<typename Ty>
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count, const Ty& val) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
while (count--) {
buffer[count] = val;
}
}
template<typename Ty>
vector<Ty>::vector(const vector& other) : buffer(new Ty[other.capacity()]), m_first(buffer), m_last(buffer + other.size()), m_end(buffer + other.capacity()) {
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
}
template<typename Ty>
vector<Ty>::vector(vector&& other) : buffer(other.buffer), m_first(other.m_first), m_last(other.m_last), m_end(other.m_end) {
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
}
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
template<typename Ty>
size_t vector<Ty>::size() const {
return static_cast<size_t>(m_last - m_first);
}
template<typename Ty>
size_t vector<Ty>::capacity() const {
return static_cast<size_t>(m_end - m_first);
}
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
template<typename Ty>
void vector<Ty>::push_back(Ty&& val) {
if (size() < capacity()) {
*(m_last++) = std::move(val);
return;
}
realloc(2, 2);
*(m_last++) = std::move(val);
}
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
template<typename Ty>
Ty& vector<Ty>::front() {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
const Ty& vector<Ty>::front() const {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
Ty& vector<Ty>::back() {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
const Ty& vector<Ty>::back() const {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
Ty& vector<Ty>::operator(const size_t pos) {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
const Ty& vector<Ty>::operator(const size_t pos) const {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::begin() {
return m_first;
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::end() {
return m_last;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::begin() const {
return m_first;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::end() const {
return m_last;
}
template<typename Ty>
void vector<Ty>::realloc(const size_t factor, const size_t carry) {
alloc(capacity() * factor + carry);
}
template<typename Ty>
void vector<Ty>::alloc(const size_t cap) {
Ty* new_buffer = new Ty[cap];
size_t sz = size();
for (size_t i = 0; i < sz; ++i) {
new_buffer[i] = buffer[i];
}
this->~vector();
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
}
c++ reinventing-the-wheel vectors
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
add a comment |
Here is a simple reimplementation of most of std::vector
. I want to check if I implement it correctly but the standard libraries source code is not very readable for me.
namespace nonstd {
template<typename Ty>
class vector
{
public:
using iterator = Ty * ;
using const_iterator = const Ty*;
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
private:
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
};
template<typename Ty>
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count, const Ty& val) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
while (count--) {
buffer[count] = val;
}
}
template<typename Ty>
vector<Ty>::vector(const vector& other) : buffer(new Ty[other.capacity()]), m_first(buffer), m_last(buffer + other.size()), m_end(buffer + other.capacity()) {
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
}
template<typename Ty>
vector<Ty>::vector(vector&& other) : buffer(other.buffer), m_first(other.m_first), m_last(other.m_last), m_end(other.m_end) {
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
}
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
template<typename Ty>
size_t vector<Ty>::size() const {
return static_cast<size_t>(m_last - m_first);
}
template<typename Ty>
size_t vector<Ty>::capacity() const {
return static_cast<size_t>(m_end - m_first);
}
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
template<typename Ty>
void vector<Ty>::push_back(Ty&& val) {
if (size() < capacity()) {
*(m_last++) = std::move(val);
return;
}
realloc(2, 2);
*(m_last++) = std::move(val);
}
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
template<typename Ty>
Ty& vector<Ty>::front() {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
const Ty& vector<Ty>::front() const {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
Ty& vector<Ty>::back() {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
const Ty& vector<Ty>::back() const {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
Ty& vector<Ty>::operator(const size_t pos) {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
const Ty& vector<Ty>::operator(const size_t pos) const {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::begin() {
return m_first;
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::end() {
return m_last;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::begin() const {
return m_first;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::end() const {
return m_last;
}
template<typename Ty>
void vector<Ty>::realloc(const size_t factor, const size_t carry) {
alloc(capacity() * factor + carry);
}
template<typename Ty>
void vector<Ty>::alloc(const size_t cap) {
Ty* new_buffer = new Ty[cap];
size_t sz = size();
for (size_t i = 0; i < sz; ++i) {
new_buffer[i] = buffer[i];
}
this->~vector();
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
}
c++ reinventing-the-wheel vectors
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
You should take a look at the articles I wrote about building your own vector: lokiastari.com/series The main improvement I think you need to make isLazy Construction of elements
which requires you to use placement new. See: lokiastari.com/blog/2016/02/27/vector/index.html
– Martin York
4 hours ago
Note: If you manually run the destructor of the objectthis->~vector();
then the object is no longer valid until you re-run the constructor. Since you are inside the object you would need to call the constructor via placement new.
– Martin York
4 hours ago
@MartinYork Thank you, your articles are awesome! Will try again and post it here.
– Einiemand
3 hours ago
add a comment |
Here is a simple reimplementation of most of std::vector
. I want to check if I implement it correctly but the standard libraries source code is not very readable for me.
namespace nonstd {
template<typename Ty>
class vector
{
public:
using iterator = Ty * ;
using const_iterator = const Ty*;
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
private:
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
};
template<typename Ty>
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count, const Ty& val) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
while (count--) {
buffer[count] = val;
}
}
template<typename Ty>
vector<Ty>::vector(const vector& other) : buffer(new Ty[other.capacity()]), m_first(buffer), m_last(buffer + other.size()), m_end(buffer + other.capacity()) {
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
}
template<typename Ty>
vector<Ty>::vector(vector&& other) : buffer(other.buffer), m_first(other.m_first), m_last(other.m_last), m_end(other.m_end) {
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
}
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
template<typename Ty>
size_t vector<Ty>::size() const {
return static_cast<size_t>(m_last - m_first);
}
template<typename Ty>
size_t vector<Ty>::capacity() const {
return static_cast<size_t>(m_end - m_first);
}
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
template<typename Ty>
void vector<Ty>::push_back(Ty&& val) {
if (size() < capacity()) {
*(m_last++) = std::move(val);
return;
}
realloc(2, 2);
*(m_last++) = std::move(val);
}
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
template<typename Ty>
Ty& vector<Ty>::front() {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
const Ty& vector<Ty>::front() const {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
Ty& vector<Ty>::back() {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
const Ty& vector<Ty>::back() const {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
Ty& vector<Ty>::operator(const size_t pos) {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
const Ty& vector<Ty>::operator(const size_t pos) const {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::begin() {
return m_first;
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::end() {
return m_last;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::begin() const {
return m_first;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::end() const {
return m_last;
}
template<typename Ty>
void vector<Ty>::realloc(const size_t factor, const size_t carry) {
alloc(capacity() * factor + carry);
}
template<typename Ty>
void vector<Ty>::alloc(const size_t cap) {
Ty* new_buffer = new Ty[cap];
size_t sz = size();
for (size_t i = 0; i < sz; ++i) {
new_buffer[i] = buffer[i];
}
this->~vector();
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
}
c++ reinventing-the-wheel vectors
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Here is a simple reimplementation of most of std::vector
. I want to check if I implement it correctly but the standard libraries source code is not very readable for me.
namespace nonstd {
template<typename Ty>
class vector
{
public:
using iterator = Ty * ;
using const_iterator = const Ty*;
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
private:
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
};
template<typename Ty>
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
}
template<typename Ty>
vector<Ty>::vector(const size_t count, const Ty& val) : buffer(new Ty[count]), m_first(buffer), m_last(buffer + count), m_end(buffer + count) {
while (count--) {
buffer[count] = val;
}
}
template<typename Ty>
vector<Ty>::vector(const vector& other) : buffer(new Ty[other.capacity()]), m_first(buffer), m_last(buffer + other.size()), m_end(buffer + other.capacity()) {
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
}
template<typename Ty>
vector<Ty>::vector(vector&& other) : buffer(other.buffer), m_first(other.m_first), m_last(other.m_last), m_end(other.m_end) {
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
}
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
template<typename Ty>
size_t vector<Ty>::size() const {
return static_cast<size_t>(m_last - m_first);
}
template<typename Ty>
size_t vector<Ty>::capacity() const {
return static_cast<size_t>(m_end - m_first);
}
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
template<typename Ty>
void vector<Ty>::push_back(Ty&& val) {
if (size() < capacity()) {
*(m_last++) = std::move(val);
return;
}
realloc(2, 2);
*(m_last++) = std::move(val);
}
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
template<typename Ty>
Ty& vector<Ty>::front() {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
const Ty& vector<Ty>::front() const {
if (size() == 0) {
throw std::exception("front(): vector is empty");
}
return *begin();
}
template<typename Ty>
Ty& vector<Ty>::back() {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
const Ty& vector<Ty>::back() const {
if (size() == 0) {
throw std::exception("back(): vector is empty");
}
return *(end() - 1);
}
template<typename Ty>
Ty& vector<Ty>::operator(const size_t pos) {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
const Ty& vector<Ty>::operator(const size_t pos) const {
if (pos >= size()) {
throw std::exception("index out of range");
}
return buffer[pos];
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::begin() {
return m_first;
}
template<typename Ty>
typename vector<Ty>::iterator vector<Ty>::end() {
return m_last;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::begin() const {
return m_first;
}
template<typename Ty>
typename vector<Ty>::const_iterator vector<Ty>::end() const {
return m_last;
}
template<typename Ty>
void vector<Ty>::realloc(const size_t factor, const size_t carry) {
alloc(capacity() * factor + carry);
}
template<typename Ty>
void vector<Ty>::alloc(const size_t cap) {
Ty* new_buffer = new Ty[cap];
size_t sz = size();
for (size_t i = 0; i < sz; ++i) {
new_buffer[i] = buffer[i];
}
this->~vector();
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
}
c++ reinventing-the-wheel vectors
c++ reinventing-the-wheel vectors
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited 10 hours ago
Deduplicator
11.1k1849
11.1k1849
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 19 hours ago
EiniemandEiniemand
433
433
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Einiemand is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
You should take a look at the articles I wrote about building your own vector: lokiastari.com/series The main improvement I think you need to make isLazy Construction of elements
which requires you to use placement new. See: lokiastari.com/blog/2016/02/27/vector/index.html
– Martin York
4 hours ago
Note: If you manually run the destructor of the objectthis->~vector();
then the object is no longer valid until you re-run the constructor. Since you are inside the object you would need to call the constructor via placement new.
– Martin York
4 hours ago
@MartinYork Thank you, your articles are awesome! Will try again and post it here.
– Einiemand
3 hours ago
add a comment |
You should take a look at the articles I wrote about building your own vector: lokiastari.com/series The main improvement I think you need to make isLazy Construction of elements
which requires you to use placement new. See: lokiastari.com/blog/2016/02/27/vector/index.html
– Martin York
4 hours ago
Note: If you manually run the destructor of the objectthis->~vector();
then the object is no longer valid until you re-run the constructor. Since you are inside the object you would need to call the constructor via placement new.
– Martin York
4 hours ago
@MartinYork Thank you, your articles are awesome! Will try again and post it here.
– Einiemand
3 hours ago
You should take a look at the articles I wrote about building your own vector: lokiastari.com/series The main improvement I think you need to make is
Lazy Construction of elements
which requires you to use placement new. See: lokiastari.com/blog/2016/02/27/vector/index.html– Martin York
4 hours ago
You should take a look at the articles I wrote about building your own vector: lokiastari.com/series The main improvement I think you need to make is
Lazy Construction of elements
which requires you to use placement new. See: lokiastari.com/blog/2016/02/27/vector/index.html– Martin York
4 hours ago
Note: If you manually run the destructor of the object
this->~vector();
then the object is no longer valid until you re-run the constructor. Since you are inside the object you would need to call the constructor via placement new.– Martin York
4 hours ago
Note: If you manually run the destructor of the object
this->~vector();
then the object is no longer valid until you re-run the constructor. Since you are inside the object you would need to call the constructor via placement new.– Martin York
4 hours ago
@MartinYork Thank you, your articles are awesome! Will try again and post it here.
– Einiemand
3 hours ago
@MartinYork Thank you, your articles are awesome! Will try again and post it here.
– Einiemand
3 hours ago
add a comment |
3 Answers
3
active
oldest
votes
Code:
I realise you're not implementing the complete functionality, but you probably want add and use at least the
value_type
,size_type
,reference
andconst_reference
typedefs.operator==
andoperator!=
are simple to implement and quite useful.Use
std::size_t
, notsize_t
(the latter is the C version).Prefer named constants to magic numbers (e.g. declare a
static const std::size_t m_initial_size = 10u;
).
m_first
is the same asbuffer
, so we don't need it.It's simpler to store the
size
andcapacity
, and calculatelast
andend
when needed (we seem to need the size and capacity quite often).
- Good job with
const
-correctness. Note that function arguments passed by value may be better left non-const
because:
const
is very important for reference / pointer types since with these the function can affect a variable outside its scope. It's easier to read a function definition where these "important"const
s stand out. e.g.(int foo, T const* bar)
vs(const int foo, T const* const bar)
.- C++ will actually match a function declaration that includes / omits a
const
for a pass-by-value argument with a function definition that omits / includes it respectively.
Note that the standard version of the single argument constructor
explicit vector(const size_t count);
does initialize elements (as if it were callingvector(const size_t count, const Ty& val);
with a default constructedTy
). In fact, we can use a delegating constructor to do this:
template<typename Ty>
vector<Ty>::vector(const size_t count) : vector(count, Ty()) { }
- Don't call the class destructor in the copy / move operators. The destructor should be called only once (usually automatically) when a class is destroyed. Calling it multiple times may be undefined behaviour. The memory cleanup should be moved into a separate function (e.g.
deallocate()
), which should be called wherever necessary.
Design:
The difference between memory allocation and object construction is an important feature of std::vector
that isn't mirrored completely in this nonstd
version. With std::vector
, a memory buffer of the appropriate size is allocated without any constructor calls. Then objects are constructed in place inside this buffer using "placement new".
As such, it might be cleaner to abstract the memory buffer into a separate class that only allocates / deallocates memory. This is separate from the construction, copying, and destruction of the elements contained within this buffer, which can be handled by the vector
class.
Various standard algorithms also exist to help with placement new, e.g. std::uninitialized_copy_n
and std::uninitialized_fill_n
.
As a (largely untested) example:
#include <cassert>
#include <cstddef>
#include <algorithm>
namespace nonstd {
template<class Ty>
class memory_block
{
public:
using size_type = std::size_t;
using value_type = Ty;
using pointer = Ty*;
using const_pointer = Ty const*;
memory_block():
m_size(size_type{ 0 }),
m_buffer(nullptr) { }
explicit memory_block(size_type count):
m_size(count),
m_buffer(allocate(m_size)) { }
memory_block(memory_block const& other):
m_size(other.m_size),
m_buffer(allcoate(m_size)) { }
memory_block(memory_block&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer))
{
other.m_size = size_type{ 0 };
other.m_buffer = nullptr;
}
~memory_block()
{
deallocate(m_buffer);
}
void swap(memory_block& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
memory_block& operator=(memory_block const& other)
{
auto temp = memory_block(other);
swap(temp);
return *this;
}
memory_block& operator=(memory_block&& other)
{
auto temp = memory_block(std::move(other));
swap(temp);
return *this;
}
size_type size() const
{
return m_size;
}
pointer data()
{
return m_buffer;
}
const_pointer data() const
{
return m_buffer;
}
pointer at(size_type index)
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
const_pointer at(size_type index) const
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
private:
static pointer allocate(std::size_t size)
{
return static_cast<pointer>(::operator new (sizeof(value_type) * size));
}
static void deallocate(pointer buffer)
{
delete static_cast<void*>(buffer);
}
std::size_t m_size;
Ty* m_buffer;
};
template<class Ty>
inline void swap(memory_block<Ty>& a, memory_block<Ty>& b)
{
a.swap(b);
}
template<class Ty>
class vector
{
public:
using size_type = std::size_t;
using value_type = Ty;
vector();
explicit vector(size_type count);
vector(size_type count, const value_type& val);
vector(const vector& other);
vector(vector&& other);
~vector();
void swap(vector& other);
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const value_type& val);
void pop_back();
private:
static const size_type M_INITIAL_SIZE = size_type{ 10 };
size_type m_size;
memory_block<Ty> m_buffer;
void grow(size_type amount);
void reallocate(size_type min_size);
void construct(size_type index, const value_type& value);
void destruct(size_type index);
void destruct_all();
};
template<class Ty>
inline void swap(vector<Ty>& a, vector<Ty>& b)
{
a.swap(b);
}
template<class Ty>
vector<Ty>::vector():
m_size(0u),
m_buffer(M_INITIAL_SIZE)
{
}
template<class Ty>
vector<Ty>::vector(size_type count):
m_size(count),
m_buffer(m_size)
{
std::uninitialized_value_construct_n(m_buffer.data(), m_size); // value construct each element w/ placement new (C++17)
}
template<class Ty>
vector<Ty>::vector(size_type count, const value_type& value) :
m_size(count),
m_buffer(m_size)
{
std::uninitialized_fill_n(m_buffer.data(), m_size, value); // copy construct each element w/ placement new
}
template<class Ty>
vector<Ty>::vector(const vector& other):
m_size(other.m_size),
m_buffer(m_size) // note: allocates only what we need to contain the existing elements, not the same as the capacity of the other buffer
{
std::uninitialized_copy_n(other.m_buffer.data(), other.m_size, m_buffer.data()); // copy construct each element from old buffer to new buffer w/ placement new
}
template<class Ty>
vector<Ty>::vector(vector&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer)) // take ownership of the buffer
{
other.m_size = size_type{ 0 }; // other vector is now empty (nothing needs to be constructed / destructed)
}
template<class Ty>
vector<Ty>::~vector()
{
destruct_all();
}
template<class Ty>
void vector<Ty>::swap(vector& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(const vector& other)
{
auto temp = vector(other);
swap(temp);
return *this;
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(vector&& other)
{
auto temp = vector(std::move(other));
swap(temp);
return *this;
}
template<class Ty>
size_t vector<Ty>::size() const
{
return m_size;
}
template<class Ty>
size_t vector<Ty>::capacity() const
{
return m_buffer.size();
}
template<class Ty>
void vector<Ty>::push_back(const value_type& value)
{
grow(size_type{ 1 });
construct(m_size, value);
++m_size;
}
template<class Ty>
void vector<Ty>::pop_back()
{
assert(m_size > 0); // maybe throw instead
destruct(m_size - 1);
--m_size;
}
template<class Ty>
void vector<Ty>::grow(size_type amount)
{
if (m_buffer.size() - m_size < amount)
reallocate(m_size + amount);
}
template<class Ty>
void vector<Ty>::reallocate(size_type min_size)
{
assert(min_size > m_size);
auto capacity = std::max(min_size, m_buffer.size() + std::max(m_buffer.size() / size_type{ 2 }, size_type{ 1 })); // growth factor of 1.5ish
auto buffer = memory_block<value_type>(capacity);
std::uninitialized_copy_n(m_buffer.data(), m_size, buffer.data()); // copy each element from old buffer to new buffer w/ placement new
destruct_all(); // clean up the old buffer (call destructors on each of the old elements)
m_buffer = std::move(buffer); // take ownership of the new buffer
}
template<class Ty>
void vector<Ty>::construct(size_type index, const value_type& value)
{
new (m_buffer.at(index)) value_type(value); // placement new w/ copy constructor
}
template<class Ty>
void vector<Ty>::destruct(size_type index)
{
assert(index < m_size);
m_buffer.at(index)->~value_type(); // explictly call destructor (because we used placement new)
}
template<class Ty>
void vector<Ty>::destruct_all()
{
for (auto index = size_type{ 0 }; index != m_size; ++index)
destruct(index);
}
} // nonstd
int main()
{
{
nonstd::vector<int> v;
v.push_back(10);
}
{
nonstd::vector<int> v(5);
v.pop_back();
}
{
nonstd::vector<int> v(5, 1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(v1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(std::move(v1));
}
}
Note the use of the "copy and swap" idiom in the copy and move assignment operators, which makes implementing them quite a lot easier.
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
add a comment |
Undefined type
We use size_t
with no definition. It seems like we want to use std::size_t
, in which case, we need a suitable include, such as <cstddef>
.
We also need to include <utility>
for std::move()
and <stdexcept>
for std::exception
.
Missing types
This template accepts only one argument, but a standard vector also has an Allocator
template argument (which defaults to std::allocator<T>
). There's quite a lot that will need to be changed to accept and use a provided allocator class.
std::vector
must provide member types value_type
, allocator_type
, size_type
, difference_type
, reference
, const_reference
, pointer
, const_pointer
, reverse_iterator
and const_reverse_iterator
, but these are all missing from this implementation.
Missing member functions
There's no public assign()
, at()
, data()
, empty()
, max_size()
, reserve()
, shrink_to_fit()
, clear()
, insert()
, emplace()
, erase()
, emplace_back()
, resize()
or swap()
members. Were also missing the const/reverse versions of begin()
and end()
, such as rbegin()
and crend()
.
Constructors and assignment
We're missing the initializer-list versions of the constructor and assignment operator.
The line lengths are very long - consider using a separate line for each initializer, like this:
template<typename Ty>
vector<Ty>::vector()
: buffer(new Ty[10]),
m_first(buffer),
m_last(buffer),
m_end(buffer + 10)
{
}
I'm not sure where the magic number 10
comes from in the above - it's probably worth defining a private constant default_initial_capacity
for this.
The constructor that accepts a single count
argument fails to initialize its elements (std::vector<int>(5)
will create a vector containing five zeros, but our equivalent will have five uninitialized values, which may well cause bugs). This could be avoided by delegating to vector(count, T{})
. We should also check for a size of zero and either avoid allocating, or round up to our default capacity in that case.
The (count, val)
constructor won't compile, as it attempts to modify const count
. We could make count
mutable, but I think we should simply use std::fill()
(from <algorithm>
):
template<typename Ty>
vector<Ty>::vector(const std::size_t count, const Ty& val)
: buffer(new Ty[count]),
m_first(buffer),
m_last(buffer + count),
m_end(buffer + count)
{
std::fill(m_first, m_last, val);
}
Copy constructor could usefully shrink to fit, using the size rather than the capacity of the source vector to determine the new vector's capacity:
template<typename Ty>
vector<Ty>::vector(const vector& other)
: buffer(new Ty[other.size()]),
m_first(buffer),
m_last(buffer + other.size()),
m_end(m_last)
{
std::copy(other.m_first, other.m_last, m_first);
}
Again, I use a standard algorithm to avoid hand-coding my own loop.
Move-construction and move-assignment are most easily implemented using swap()
:
template<typename Ty>
vector<Ty>::vector(vector&& other)
: vector()
{
swap(other);
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other)
{
swap(other);
return *this;
}
It certainly looks wrong to explicitly call the destructor in the assignment operator. All we're using it for is to delete buffer
, so just do that. Better still, use the copy-and-swap idiom:
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other)
{
swap(vector<Ty>(other)); // copy and swap
return *this;
}
Destructor
There's no need to assign nullptr
to the members - they are about to go out of scope, so can't be accessed after the destructor finishes. Also, there's no need to test that buffer
is non-null, for two reasons: first, our logic never allows a non-null buffer to exist, and secondly, delete
will happily do nothing if its argument is null.
Modifiers
Look at push_back()
:
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
See how *(m_last++) = val;
is common to both paths? We can reorder the test so that we don't duplicate that; to my eyes at least, that makes a more natural expression ("ensure there's room, then add the element"):
template<typename Ty>
void vector<Ty>::push_back(const Ty& val)
{
if (capacity() <= size()) {
realloc(2, 2);
}
*(m_last++) = val;
}
Once rbegin()
is implemented, back()
can be re-written to use that rather than doing arithmetic on the result of end()
.
Exceptions
std::exception
has no constructor that accepts a string literal - we need to use a more specific sub-class, such as std::out_of_range
. We should consider whether we have range-checking at all, outside of methods such as at()
which mandate it - standard C++ practice is to impose minimum overhead unless it's asked for.
Private members
Mostly looks okay, though alloc
could be improved using std::move()
algorithm instead of a hand-written copy loop, and simple delete buffer
instead of calling destructor, as described above. It's also wise to allow this function to shrink the buffer; we'll need that for some of the as-yet-unimplemented code:
template<typename Ty>
void vector<Ty>::alloc(const std::size_t cap) {
Ty* new_buffer = new Ty[cap];
auto sz = size();
if (sz < cap) {
sz = cap;
}
std::move(m_first, m_first + sz, new_buffer);
delete buffer;
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
Redundant member
Is there any need for separate buffer
and m_first
members? They have the same type, and as far as I can see, they always hold the same value.
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
that's why they have replaced one ctorvector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11
– RiaD
7 hours ago
You're right @RiaD. I was misremembering, and thought thatvalue_type
needed to beDefaultConstructible
.
– Toby Speight
7 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
add a comment |
- Missing includes:
- You need an include for
std::size_t
(though you could just usesize_type
everywhere, and declare that asusing size_type = decltype(sizeof 0);
. - You use
std::move
, and thus need<utility>
.
- You need an include for
namespace nonstd {
template<typename Ty>
class vector
If (nearly) everything is in the same namespace, it is conventional not to indent it.
Obviously, you forego allocator-support (for now?).
Using
Ty
instead ofT
is very unconventional. Following convention reduces cognitive load, allowing you to expend it elsewhere more profitably. I'll pretend you followed convention for the rest of the review.
using iterator = Ty * ;
using const_iterator = const Ty*;
The whitespace around the first
*
is misplaced. Consider auto-formatting, or just be a bit more careful to stay consistent.Those aliases are good. You are missing myriad other useful (and necessary) ones though, namely
value_type
,size_type
,difference_type
,reference
,const_reference
,pointer
, andconst_pointer
.
And when you add reverse-iterator-support,reverse_iterator
andconst_reverse_iterator
.
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
There is no reason not to make the default-ctor
noexcept
andconstexpr
, even if that means not allocating any memory. Especially as passing a zero count to a ctor or invoking the move-ctor can already result in a 0-capacity vector.
Also, the magic constant you chose is pretty arbitrary, and should be a (private) named constant if you decide to keep it.Having
const
parameters in an API is at best irritating to the reader: It doesn't actually do anything.You are missing constructors from
std::initializer_list<T>
and iterator-pair. At least copy-ctor and creating from initializer-list should then delegate to iterator-pair for code-reuse.The move-ctor cannot throw by design, and should thus be marked
noexcept
.Dito for move-assignment.
size_t size() const;
size_t capacity() const;
Both observers above could be
constexpr
if you have at least oneconstexpr
ctor...You are missing
empty()
, andmax_size()
.
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
You are missing
emplace_back()
, which the first two should delegate to.Also missing are
clear()
,insert()
,emplace()
,erase()
, andswap()
. The last one is crucial for a simple, efficient and exception-safe implementation of much of the class.
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
All the mutable versions can be implemented by delegating to the
const
variant and applying a judiciousconst_cast
to the result.
Especially as you decided that "undefined behavior" means "throws exception" in your case.Even though you gave the above observers the behavior of
at()
, you shouldn't leave it out.You are missing
data()
completely. Yes, it behaves the same asbegin()
for you, but there you have it.
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
- You are missing the reverse_iterator equivalents.
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
- Either
buffer
orm_first
is redundant.
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
Centralizing allocation to enforce policy and reduce repetition is a good idea. You aren't quite there though, and implementing the wrong signature. You need one private member for automatic growth
void grow(size_type n = 1)
and the rest should be public, so a consumer who knows can use it:
void reserve(size_type n);
void resize(size_type n);
void shrink_to_fit();
Your current allocation-policy is "double plus constant" on reallocation. That's actually the worst one you could choose, as it makes it impossible to reuse any memory over multiple consecutive reallocations (all the returned memory from earlier together is never enough). For that, it should be below two, maybe
m_new = max(needed, m_old * 7 / 4)
for automatic reallocation.You are missing assignment from
std::initializer_list<T>
, the more versatileassign()
,
Now only implementation-details:
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
// and many more
- You are aware that
new T[n]
doesn't only reserve space forn
T
s, but also constructs them? That extra-work might be quite involved, and is most emphatically not expected. You should only allocate raw memory (by calling e. g.void* operator new(std::size_t)
) and construct the members on demand as needed.
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
- Why do you guard against deleting a nullpointer? Also, why repaint the house when demolishing it?
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
Don't pessimise the common case, by optimising self-assignment.
Calling the dtor means the lifetime ended. Pretending it didn't causes Undefined Behavior.
Assignment should be transactional: Either succeed, or have no effect. Look into the copy-and-swap idiom.
Move-assignment should be simple and efficient: Just
swap()
.
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
pop_back()
seems to be the only part ofvector
assuming you construct and destruct elements on demand. That mismatch can be explosive.
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Einiemand is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211241%2fsimple-re-implementation-of-stdvector%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
Code:
I realise you're not implementing the complete functionality, but you probably want add and use at least the
value_type
,size_type
,reference
andconst_reference
typedefs.operator==
andoperator!=
are simple to implement and quite useful.Use
std::size_t
, notsize_t
(the latter is the C version).Prefer named constants to magic numbers (e.g. declare a
static const std::size_t m_initial_size = 10u;
).
m_first
is the same asbuffer
, so we don't need it.It's simpler to store the
size
andcapacity
, and calculatelast
andend
when needed (we seem to need the size and capacity quite often).
- Good job with
const
-correctness. Note that function arguments passed by value may be better left non-const
because:
const
is very important for reference / pointer types since with these the function can affect a variable outside its scope. It's easier to read a function definition where these "important"const
s stand out. e.g.(int foo, T const* bar)
vs(const int foo, T const* const bar)
.- C++ will actually match a function declaration that includes / omits a
const
for a pass-by-value argument with a function definition that omits / includes it respectively.
Note that the standard version of the single argument constructor
explicit vector(const size_t count);
does initialize elements (as if it were callingvector(const size_t count, const Ty& val);
with a default constructedTy
). In fact, we can use a delegating constructor to do this:
template<typename Ty>
vector<Ty>::vector(const size_t count) : vector(count, Ty()) { }
- Don't call the class destructor in the copy / move operators. The destructor should be called only once (usually automatically) when a class is destroyed. Calling it multiple times may be undefined behaviour. The memory cleanup should be moved into a separate function (e.g.
deallocate()
), which should be called wherever necessary.
Design:
The difference between memory allocation and object construction is an important feature of std::vector
that isn't mirrored completely in this nonstd
version. With std::vector
, a memory buffer of the appropriate size is allocated without any constructor calls. Then objects are constructed in place inside this buffer using "placement new".
As such, it might be cleaner to abstract the memory buffer into a separate class that only allocates / deallocates memory. This is separate from the construction, copying, and destruction of the elements contained within this buffer, which can be handled by the vector
class.
Various standard algorithms also exist to help with placement new, e.g. std::uninitialized_copy_n
and std::uninitialized_fill_n
.
As a (largely untested) example:
#include <cassert>
#include <cstddef>
#include <algorithm>
namespace nonstd {
template<class Ty>
class memory_block
{
public:
using size_type = std::size_t;
using value_type = Ty;
using pointer = Ty*;
using const_pointer = Ty const*;
memory_block():
m_size(size_type{ 0 }),
m_buffer(nullptr) { }
explicit memory_block(size_type count):
m_size(count),
m_buffer(allocate(m_size)) { }
memory_block(memory_block const& other):
m_size(other.m_size),
m_buffer(allcoate(m_size)) { }
memory_block(memory_block&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer))
{
other.m_size = size_type{ 0 };
other.m_buffer = nullptr;
}
~memory_block()
{
deallocate(m_buffer);
}
void swap(memory_block& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
memory_block& operator=(memory_block const& other)
{
auto temp = memory_block(other);
swap(temp);
return *this;
}
memory_block& operator=(memory_block&& other)
{
auto temp = memory_block(std::move(other));
swap(temp);
return *this;
}
size_type size() const
{
return m_size;
}
pointer data()
{
return m_buffer;
}
const_pointer data() const
{
return m_buffer;
}
pointer at(size_type index)
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
const_pointer at(size_type index) const
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
private:
static pointer allocate(std::size_t size)
{
return static_cast<pointer>(::operator new (sizeof(value_type) * size));
}
static void deallocate(pointer buffer)
{
delete static_cast<void*>(buffer);
}
std::size_t m_size;
Ty* m_buffer;
};
template<class Ty>
inline void swap(memory_block<Ty>& a, memory_block<Ty>& b)
{
a.swap(b);
}
template<class Ty>
class vector
{
public:
using size_type = std::size_t;
using value_type = Ty;
vector();
explicit vector(size_type count);
vector(size_type count, const value_type& val);
vector(const vector& other);
vector(vector&& other);
~vector();
void swap(vector& other);
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const value_type& val);
void pop_back();
private:
static const size_type M_INITIAL_SIZE = size_type{ 10 };
size_type m_size;
memory_block<Ty> m_buffer;
void grow(size_type amount);
void reallocate(size_type min_size);
void construct(size_type index, const value_type& value);
void destruct(size_type index);
void destruct_all();
};
template<class Ty>
inline void swap(vector<Ty>& a, vector<Ty>& b)
{
a.swap(b);
}
template<class Ty>
vector<Ty>::vector():
m_size(0u),
m_buffer(M_INITIAL_SIZE)
{
}
template<class Ty>
vector<Ty>::vector(size_type count):
m_size(count),
m_buffer(m_size)
{
std::uninitialized_value_construct_n(m_buffer.data(), m_size); // value construct each element w/ placement new (C++17)
}
template<class Ty>
vector<Ty>::vector(size_type count, const value_type& value) :
m_size(count),
m_buffer(m_size)
{
std::uninitialized_fill_n(m_buffer.data(), m_size, value); // copy construct each element w/ placement new
}
template<class Ty>
vector<Ty>::vector(const vector& other):
m_size(other.m_size),
m_buffer(m_size) // note: allocates only what we need to contain the existing elements, not the same as the capacity of the other buffer
{
std::uninitialized_copy_n(other.m_buffer.data(), other.m_size, m_buffer.data()); // copy construct each element from old buffer to new buffer w/ placement new
}
template<class Ty>
vector<Ty>::vector(vector&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer)) // take ownership of the buffer
{
other.m_size = size_type{ 0 }; // other vector is now empty (nothing needs to be constructed / destructed)
}
template<class Ty>
vector<Ty>::~vector()
{
destruct_all();
}
template<class Ty>
void vector<Ty>::swap(vector& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(const vector& other)
{
auto temp = vector(other);
swap(temp);
return *this;
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(vector&& other)
{
auto temp = vector(std::move(other));
swap(temp);
return *this;
}
template<class Ty>
size_t vector<Ty>::size() const
{
return m_size;
}
template<class Ty>
size_t vector<Ty>::capacity() const
{
return m_buffer.size();
}
template<class Ty>
void vector<Ty>::push_back(const value_type& value)
{
grow(size_type{ 1 });
construct(m_size, value);
++m_size;
}
template<class Ty>
void vector<Ty>::pop_back()
{
assert(m_size > 0); // maybe throw instead
destruct(m_size - 1);
--m_size;
}
template<class Ty>
void vector<Ty>::grow(size_type amount)
{
if (m_buffer.size() - m_size < amount)
reallocate(m_size + amount);
}
template<class Ty>
void vector<Ty>::reallocate(size_type min_size)
{
assert(min_size > m_size);
auto capacity = std::max(min_size, m_buffer.size() + std::max(m_buffer.size() / size_type{ 2 }, size_type{ 1 })); // growth factor of 1.5ish
auto buffer = memory_block<value_type>(capacity);
std::uninitialized_copy_n(m_buffer.data(), m_size, buffer.data()); // copy each element from old buffer to new buffer w/ placement new
destruct_all(); // clean up the old buffer (call destructors on each of the old elements)
m_buffer = std::move(buffer); // take ownership of the new buffer
}
template<class Ty>
void vector<Ty>::construct(size_type index, const value_type& value)
{
new (m_buffer.at(index)) value_type(value); // placement new w/ copy constructor
}
template<class Ty>
void vector<Ty>::destruct(size_type index)
{
assert(index < m_size);
m_buffer.at(index)->~value_type(); // explictly call destructor (because we used placement new)
}
template<class Ty>
void vector<Ty>::destruct_all()
{
for (auto index = size_type{ 0 }; index != m_size; ++index)
destruct(index);
}
} // nonstd
int main()
{
{
nonstd::vector<int> v;
v.push_back(10);
}
{
nonstd::vector<int> v(5);
v.pop_back();
}
{
nonstd::vector<int> v(5, 1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(v1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(std::move(v1));
}
}
Note the use of the "copy and swap" idiom in the copy and move assignment operators, which makes implementing them quite a lot easier.
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
add a comment |
Code:
I realise you're not implementing the complete functionality, but you probably want add and use at least the
value_type
,size_type
,reference
andconst_reference
typedefs.operator==
andoperator!=
are simple to implement and quite useful.Use
std::size_t
, notsize_t
(the latter is the C version).Prefer named constants to magic numbers (e.g. declare a
static const std::size_t m_initial_size = 10u;
).
m_first
is the same asbuffer
, so we don't need it.It's simpler to store the
size
andcapacity
, and calculatelast
andend
when needed (we seem to need the size and capacity quite often).
- Good job with
const
-correctness. Note that function arguments passed by value may be better left non-const
because:
const
is very important for reference / pointer types since with these the function can affect a variable outside its scope. It's easier to read a function definition where these "important"const
s stand out. e.g.(int foo, T const* bar)
vs(const int foo, T const* const bar)
.- C++ will actually match a function declaration that includes / omits a
const
for a pass-by-value argument with a function definition that omits / includes it respectively.
Note that the standard version of the single argument constructor
explicit vector(const size_t count);
does initialize elements (as if it were callingvector(const size_t count, const Ty& val);
with a default constructedTy
). In fact, we can use a delegating constructor to do this:
template<typename Ty>
vector<Ty>::vector(const size_t count) : vector(count, Ty()) { }
- Don't call the class destructor in the copy / move operators. The destructor should be called only once (usually automatically) when a class is destroyed. Calling it multiple times may be undefined behaviour. The memory cleanup should be moved into a separate function (e.g.
deallocate()
), which should be called wherever necessary.
Design:
The difference between memory allocation and object construction is an important feature of std::vector
that isn't mirrored completely in this nonstd
version. With std::vector
, a memory buffer of the appropriate size is allocated without any constructor calls. Then objects are constructed in place inside this buffer using "placement new".
As such, it might be cleaner to abstract the memory buffer into a separate class that only allocates / deallocates memory. This is separate from the construction, copying, and destruction of the elements contained within this buffer, which can be handled by the vector
class.
Various standard algorithms also exist to help with placement new, e.g. std::uninitialized_copy_n
and std::uninitialized_fill_n
.
As a (largely untested) example:
#include <cassert>
#include <cstddef>
#include <algorithm>
namespace nonstd {
template<class Ty>
class memory_block
{
public:
using size_type = std::size_t;
using value_type = Ty;
using pointer = Ty*;
using const_pointer = Ty const*;
memory_block():
m_size(size_type{ 0 }),
m_buffer(nullptr) { }
explicit memory_block(size_type count):
m_size(count),
m_buffer(allocate(m_size)) { }
memory_block(memory_block const& other):
m_size(other.m_size),
m_buffer(allcoate(m_size)) { }
memory_block(memory_block&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer))
{
other.m_size = size_type{ 0 };
other.m_buffer = nullptr;
}
~memory_block()
{
deallocate(m_buffer);
}
void swap(memory_block& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
memory_block& operator=(memory_block const& other)
{
auto temp = memory_block(other);
swap(temp);
return *this;
}
memory_block& operator=(memory_block&& other)
{
auto temp = memory_block(std::move(other));
swap(temp);
return *this;
}
size_type size() const
{
return m_size;
}
pointer data()
{
return m_buffer;
}
const_pointer data() const
{
return m_buffer;
}
pointer at(size_type index)
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
const_pointer at(size_type index) const
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
private:
static pointer allocate(std::size_t size)
{
return static_cast<pointer>(::operator new (sizeof(value_type) * size));
}
static void deallocate(pointer buffer)
{
delete static_cast<void*>(buffer);
}
std::size_t m_size;
Ty* m_buffer;
};
template<class Ty>
inline void swap(memory_block<Ty>& a, memory_block<Ty>& b)
{
a.swap(b);
}
template<class Ty>
class vector
{
public:
using size_type = std::size_t;
using value_type = Ty;
vector();
explicit vector(size_type count);
vector(size_type count, const value_type& val);
vector(const vector& other);
vector(vector&& other);
~vector();
void swap(vector& other);
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const value_type& val);
void pop_back();
private:
static const size_type M_INITIAL_SIZE = size_type{ 10 };
size_type m_size;
memory_block<Ty> m_buffer;
void grow(size_type amount);
void reallocate(size_type min_size);
void construct(size_type index, const value_type& value);
void destruct(size_type index);
void destruct_all();
};
template<class Ty>
inline void swap(vector<Ty>& a, vector<Ty>& b)
{
a.swap(b);
}
template<class Ty>
vector<Ty>::vector():
m_size(0u),
m_buffer(M_INITIAL_SIZE)
{
}
template<class Ty>
vector<Ty>::vector(size_type count):
m_size(count),
m_buffer(m_size)
{
std::uninitialized_value_construct_n(m_buffer.data(), m_size); // value construct each element w/ placement new (C++17)
}
template<class Ty>
vector<Ty>::vector(size_type count, const value_type& value) :
m_size(count),
m_buffer(m_size)
{
std::uninitialized_fill_n(m_buffer.data(), m_size, value); // copy construct each element w/ placement new
}
template<class Ty>
vector<Ty>::vector(const vector& other):
m_size(other.m_size),
m_buffer(m_size) // note: allocates only what we need to contain the existing elements, not the same as the capacity of the other buffer
{
std::uninitialized_copy_n(other.m_buffer.data(), other.m_size, m_buffer.data()); // copy construct each element from old buffer to new buffer w/ placement new
}
template<class Ty>
vector<Ty>::vector(vector&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer)) // take ownership of the buffer
{
other.m_size = size_type{ 0 }; // other vector is now empty (nothing needs to be constructed / destructed)
}
template<class Ty>
vector<Ty>::~vector()
{
destruct_all();
}
template<class Ty>
void vector<Ty>::swap(vector& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(const vector& other)
{
auto temp = vector(other);
swap(temp);
return *this;
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(vector&& other)
{
auto temp = vector(std::move(other));
swap(temp);
return *this;
}
template<class Ty>
size_t vector<Ty>::size() const
{
return m_size;
}
template<class Ty>
size_t vector<Ty>::capacity() const
{
return m_buffer.size();
}
template<class Ty>
void vector<Ty>::push_back(const value_type& value)
{
grow(size_type{ 1 });
construct(m_size, value);
++m_size;
}
template<class Ty>
void vector<Ty>::pop_back()
{
assert(m_size > 0); // maybe throw instead
destruct(m_size - 1);
--m_size;
}
template<class Ty>
void vector<Ty>::grow(size_type amount)
{
if (m_buffer.size() - m_size < amount)
reallocate(m_size + amount);
}
template<class Ty>
void vector<Ty>::reallocate(size_type min_size)
{
assert(min_size > m_size);
auto capacity = std::max(min_size, m_buffer.size() + std::max(m_buffer.size() / size_type{ 2 }, size_type{ 1 })); // growth factor of 1.5ish
auto buffer = memory_block<value_type>(capacity);
std::uninitialized_copy_n(m_buffer.data(), m_size, buffer.data()); // copy each element from old buffer to new buffer w/ placement new
destruct_all(); // clean up the old buffer (call destructors on each of the old elements)
m_buffer = std::move(buffer); // take ownership of the new buffer
}
template<class Ty>
void vector<Ty>::construct(size_type index, const value_type& value)
{
new (m_buffer.at(index)) value_type(value); // placement new w/ copy constructor
}
template<class Ty>
void vector<Ty>::destruct(size_type index)
{
assert(index < m_size);
m_buffer.at(index)->~value_type(); // explictly call destructor (because we used placement new)
}
template<class Ty>
void vector<Ty>::destruct_all()
{
for (auto index = size_type{ 0 }; index != m_size; ++index)
destruct(index);
}
} // nonstd
int main()
{
{
nonstd::vector<int> v;
v.push_back(10);
}
{
nonstd::vector<int> v(5);
v.pop_back();
}
{
nonstd::vector<int> v(5, 1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(v1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(std::move(v1));
}
}
Note the use of the "copy and swap" idiom in the copy and move assignment operators, which makes implementing them quite a lot easier.
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
add a comment |
Code:
I realise you're not implementing the complete functionality, but you probably want add and use at least the
value_type
,size_type
,reference
andconst_reference
typedefs.operator==
andoperator!=
are simple to implement and quite useful.Use
std::size_t
, notsize_t
(the latter is the C version).Prefer named constants to magic numbers (e.g. declare a
static const std::size_t m_initial_size = 10u;
).
m_first
is the same asbuffer
, so we don't need it.It's simpler to store the
size
andcapacity
, and calculatelast
andend
when needed (we seem to need the size and capacity quite often).
- Good job with
const
-correctness. Note that function arguments passed by value may be better left non-const
because:
const
is very important for reference / pointer types since with these the function can affect a variable outside its scope. It's easier to read a function definition where these "important"const
s stand out. e.g.(int foo, T const* bar)
vs(const int foo, T const* const bar)
.- C++ will actually match a function declaration that includes / omits a
const
for a pass-by-value argument with a function definition that omits / includes it respectively.
Note that the standard version of the single argument constructor
explicit vector(const size_t count);
does initialize elements (as if it were callingvector(const size_t count, const Ty& val);
with a default constructedTy
). In fact, we can use a delegating constructor to do this:
template<typename Ty>
vector<Ty>::vector(const size_t count) : vector(count, Ty()) { }
- Don't call the class destructor in the copy / move operators. The destructor should be called only once (usually automatically) when a class is destroyed. Calling it multiple times may be undefined behaviour. The memory cleanup should be moved into a separate function (e.g.
deallocate()
), which should be called wherever necessary.
Design:
The difference between memory allocation and object construction is an important feature of std::vector
that isn't mirrored completely in this nonstd
version. With std::vector
, a memory buffer of the appropriate size is allocated without any constructor calls. Then objects are constructed in place inside this buffer using "placement new".
As such, it might be cleaner to abstract the memory buffer into a separate class that only allocates / deallocates memory. This is separate from the construction, copying, and destruction of the elements contained within this buffer, which can be handled by the vector
class.
Various standard algorithms also exist to help with placement new, e.g. std::uninitialized_copy_n
and std::uninitialized_fill_n
.
As a (largely untested) example:
#include <cassert>
#include <cstddef>
#include <algorithm>
namespace nonstd {
template<class Ty>
class memory_block
{
public:
using size_type = std::size_t;
using value_type = Ty;
using pointer = Ty*;
using const_pointer = Ty const*;
memory_block():
m_size(size_type{ 0 }),
m_buffer(nullptr) { }
explicit memory_block(size_type count):
m_size(count),
m_buffer(allocate(m_size)) { }
memory_block(memory_block const& other):
m_size(other.m_size),
m_buffer(allcoate(m_size)) { }
memory_block(memory_block&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer))
{
other.m_size = size_type{ 0 };
other.m_buffer = nullptr;
}
~memory_block()
{
deallocate(m_buffer);
}
void swap(memory_block& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
memory_block& operator=(memory_block const& other)
{
auto temp = memory_block(other);
swap(temp);
return *this;
}
memory_block& operator=(memory_block&& other)
{
auto temp = memory_block(std::move(other));
swap(temp);
return *this;
}
size_type size() const
{
return m_size;
}
pointer data()
{
return m_buffer;
}
const_pointer data() const
{
return m_buffer;
}
pointer at(size_type index)
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
const_pointer at(size_type index) const
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
private:
static pointer allocate(std::size_t size)
{
return static_cast<pointer>(::operator new (sizeof(value_type) * size));
}
static void deallocate(pointer buffer)
{
delete static_cast<void*>(buffer);
}
std::size_t m_size;
Ty* m_buffer;
};
template<class Ty>
inline void swap(memory_block<Ty>& a, memory_block<Ty>& b)
{
a.swap(b);
}
template<class Ty>
class vector
{
public:
using size_type = std::size_t;
using value_type = Ty;
vector();
explicit vector(size_type count);
vector(size_type count, const value_type& val);
vector(const vector& other);
vector(vector&& other);
~vector();
void swap(vector& other);
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const value_type& val);
void pop_back();
private:
static const size_type M_INITIAL_SIZE = size_type{ 10 };
size_type m_size;
memory_block<Ty> m_buffer;
void grow(size_type amount);
void reallocate(size_type min_size);
void construct(size_type index, const value_type& value);
void destruct(size_type index);
void destruct_all();
};
template<class Ty>
inline void swap(vector<Ty>& a, vector<Ty>& b)
{
a.swap(b);
}
template<class Ty>
vector<Ty>::vector():
m_size(0u),
m_buffer(M_INITIAL_SIZE)
{
}
template<class Ty>
vector<Ty>::vector(size_type count):
m_size(count),
m_buffer(m_size)
{
std::uninitialized_value_construct_n(m_buffer.data(), m_size); // value construct each element w/ placement new (C++17)
}
template<class Ty>
vector<Ty>::vector(size_type count, const value_type& value) :
m_size(count),
m_buffer(m_size)
{
std::uninitialized_fill_n(m_buffer.data(), m_size, value); // copy construct each element w/ placement new
}
template<class Ty>
vector<Ty>::vector(const vector& other):
m_size(other.m_size),
m_buffer(m_size) // note: allocates only what we need to contain the existing elements, not the same as the capacity of the other buffer
{
std::uninitialized_copy_n(other.m_buffer.data(), other.m_size, m_buffer.data()); // copy construct each element from old buffer to new buffer w/ placement new
}
template<class Ty>
vector<Ty>::vector(vector&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer)) // take ownership of the buffer
{
other.m_size = size_type{ 0 }; // other vector is now empty (nothing needs to be constructed / destructed)
}
template<class Ty>
vector<Ty>::~vector()
{
destruct_all();
}
template<class Ty>
void vector<Ty>::swap(vector& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(const vector& other)
{
auto temp = vector(other);
swap(temp);
return *this;
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(vector&& other)
{
auto temp = vector(std::move(other));
swap(temp);
return *this;
}
template<class Ty>
size_t vector<Ty>::size() const
{
return m_size;
}
template<class Ty>
size_t vector<Ty>::capacity() const
{
return m_buffer.size();
}
template<class Ty>
void vector<Ty>::push_back(const value_type& value)
{
grow(size_type{ 1 });
construct(m_size, value);
++m_size;
}
template<class Ty>
void vector<Ty>::pop_back()
{
assert(m_size > 0); // maybe throw instead
destruct(m_size - 1);
--m_size;
}
template<class Ty>
void vector<Ty>::grow(size_type amount)
{
if (m_buffer.size() - m_size < amount)
reallocate(m_size + amount);
}
template<class Ty>
void vector<Ty>::reallocate(size_type min_size)
{
assert(min_size > m_size);
auto capacity = std::max(min_size, m_buffer.size() + std::max(m_buffer.size() / size_type{ 2 }, size_type{ 1 })); // growth factor of 1.5ish
auto buffer = memory_block<value_type>(capacity);
std::uninitialized_copy_n(m_buffer.data(), m_size, buffer.data()); // copy each element from old buffer to new buffer w/ placement new
destruct_all(); // clean up the old buffer (call destructors on each of the old elements)
m_buffer = std::move(buffer); // take ownership of the new buffer
}
template<class Ty>
void vector<Ty>::construct(size_type index, const value_type& value)
{
new (m_buffer.at(index)) value_type(value); // placement new w/ copy constructor
}
template<class Ty>
void vector<Ty>::destruct(size_type index)
{
assert(index < m_size);
m_buffer.at(index)->~value_type(); // explictly call destructor (because we used placement new)
}
template<class Ty>
void vector<Ty>::destruct_all()
{
for (auto index = size_type{ 0 }; index != m_size; ++index)
destruct(index);
}
} // nonstd
int main()
{
{
nonstd::vector<int> v;
v.push_back(10);
}
{
nonstd::vector<int> v(5);
v.pop_back();
}
{
nonstd::vector<int> v(5, 1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(v1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(std::move(v1));
}
}
Note the use of the "copy and swap" idiom in the copy and move assignment operators, which makes implementing them quite a lot easier.
Code:
I realise you're not implementing the complete functionality, but you probably want add and use at least the
value_type
,size_type
,reference
andconst_reference
typedefs.operator==
andoperator!=
are simple to implement and quite useful.Use
std::size_t
, notsize_t
(the latter is the C version).Prefer named constants to magic numbers (e.g. declare a
static const std::size_t m_initial_size = 10u;
).
m_first
is the same asbuffer
, so we don't need it.It's simpler to store the
size
andcapacity
, and calculatelast
andend
when needed (we seem to need the size and capacity quite often).
- Good job with
const
-correctness. Note that function arguments passed by value may be better left non-const
because:
const
is very important for reference / pointer types since with these the function can affect a variable outside its scope. It's easier to read a function definition where these "important"const
s stand out. e.g.(int foo, T const* bar)
vs(const int foo, T const* const bar)
.- C++ will actually match a function declaration that includes / omits a
const
for a pass-by-value argument with a function definition that omits / includes it respectively.
Note that the standard version of the single argument constructor
explicit vector(const size_t count);
does initialize elements (as if it were callingvector(const size_t count, const Ty& val);
with a default constructedTy
). In fact, we can use a delegating constructor to do this:
template<typename Ty>
vector<Ty>::vector(const size_t count) : vector(count, Ty()) { }
- Don't call the class destructor in the copy / move operators. The destructor should be called only once (usually automatically) when a class is destroyed. Calling it multiple times may be undefined behaviour. The memory cleanup should be moved into a separate function (e.g.
deallocate()
), which should be called wherever necessary.
Design:
The difference between memory allocation and object construction is an important feature of std::vector
that isn't mirrored completely in this nonstd
version. With std::vector
, a memory buffer of the appropriate size is allocated without any constructor calls. Then objects are constructed in place inside this buffer using "placement new".
As such, it might be cleaner to abstract the memory buffer into a separate class that only allocates / deallocates memory. This is separate from the construction, copying, and destruction of the elements contained within this buffer, which can be handled by the vector
class.
Various standard algorithms also exist to help with placement new, e.g. std::uninitialized_copy_n
and std::uninitialized_fill_n
.
As a (largely untested) example:
#include <cassert>
#include <cstddef>
#include <algorithm>
namespace nonstd {
template<class Ty>
class memory_block
{
public:
using size_type = std::size_t;
using value_type = Ty;
using pointer = Ty*;
using const_pointer = Ty const*;
memory_block():
m_size(size_type{ 0 }),
m_buffer(nullptr) { }
explicit memory_block(size_type count):
m_size(count),
m_buffer(allocate(m_size)) { }
memory_block(memory_block const& other):
m_size(other.m_size),
m_buffer(allcoate(m_size)) { }
memory_block(memory_block&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer))
{
other.m_size = size_type{ 0 };
other.m_buffer = nullptr;
}
~memory_block()
{
deallocate(m_buffer);
}
void swap(memory_block& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
memory_block& operator=(memory_block const& other)
{
auto temp = memory_block(other);
swap(temp);
return *this;
}
memory_block& operator=(memory_block&& other)
{
auto temp = memory_block(std::move(other));
swap(temp);
return *this;
}
size_type size() const
{
return m_size;
}
pointer data()
{
return m_buffer;
}
const_pointer data() const
{
return m_buffer;
}
pointer at(size_type index)
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
const_pointer at(size_type index) const
{
assert(index < m_size); // maybe throw instead
return m_buffer + index;
}
private:
static pointer allocate(std::size_t size)
{
return static_cast<pointer>(::operator new (sizeof(value_type) * size));
}
static void deallocate(pointer buffer)
{
delete static_cast<void*>(buffer);
}
std::size_t m_size;
Ty* m_buffer;
};
template<class Ty>
inline void swap(memory_block<Ty>& a, memory_block<Ty>& b)
{
a.swap(b);
}
template<class Ty>
class vector
{
public:
using size_type = std::size_t;
using value_type = Ty;
vector();
explicit vector(size_type count);
vector(size_type count, const value_type& val);
vector(const vector& other);
vector(vector&& other);
~vector();
void swap(vector& other);
vector& operator=(const vector& other);
vector& operator=(vector&& other);
size_t size() const;
size_t capacity() const;
void push_back(const value_type& val);
void pop_back();
private:
static const size_type M_INITIAL_SIZE = size_type{ 10 };
size_type m_size;
memory_block<Ty> m_buffer;
void grow(size_type amount);
void reallocate(size_type min_size);
void construct(size_type index, const value_type& value);
void destruct(size_type index);
void destruct_all();
};
template<class Ty>
inline void swap(vector<Ty>& a, vector<Ty>& b)
{
a.swap(b);
}
template<class Ty>
vector<Ty>::vector():
m_size(0u),
m_buffer(M_INITIAL_SIZE)
{
}
template<class Ty>
vector<Ty>::vector(size_type count):
m_size(count),
m_buffer(m_size)
{
std::uninitialized_value_construct_n(m_buffer.data(), m_size); // value construct each element w/ placement new (C++17)
}
template<class Ty>
vector<Ty>::vector(size_type count, const value_type& value) :
m_size(count),
m_buffer(m_size)
{
std::uninitialized_fill_n(m_buffer.data(), m_size, value); // copy construct each element w/ placement new
}
template<class Ty>
vector<Ty>::vector(const vector& other):
m_size(other.m_size),
m_buffer(m_size) // note: allocates only what we need to contain the existing elements, not the same as the capacity of the other buffer
{
std::uninitialized_copy_n(other.m_buffer.data(), other.m_size, m_buffer.data()); // copy construct each element from old buffer to new buffer w/ placement new
}
template<class Ty>
vector<Ty>::vector(vector&& other):
m_size(std::move(other.m_size)),
m_buffer(std::move(other.m_buffer)) // take ownership of the buffer
{
other.m_size = size_type{ 0 }; // other vector is now empty (nothing needs to be constructed / destructed)
}
template<class Ty>
vector<Ty>::~vector()
{
destruct_all();
}
template<class Ty>
void vector<Ty>::swap(vector& other)
{
using std::swap;
swap(m_size, other.m_size);
swap(m_buffer, other.m_buffer);
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(const vector& other)
{
auto temp = vector(other);
swap(temp);
return *this;
}
template<class Ty>
vector<Ty>& vector<Ty>::operator=(vector&& other)
{
auto temp = vector(std::move(other));
swap(temp);
return *this;
}
template<class Ty>
size_t vector<Ty>::size() const
{
return m_size;
}
template<class Ty>
size_t vector<Ty>::capacity() const
{
return m_buffer.size();
}
template<class Ty>
void vector<Ty>::push_back(const value_type& value)
{
grow(size_type{ 1 });
construct(m_size, value);
++m_size;
}
template<class Ty>
void vector<Ty>::pop_back()
{
assert(m_size > 0); // maybe throw instead
destruct(m_size - 1);
--m_size;
}
template<class Ty>
void vector<Ty>::grow(size_type amount)
{
if (m_buffer.size() - m_size < amount)
reallocate(m_size + amount);
}
template<class Ty>
void vector<Ty>::reallocate(size_type min_size)
{
assert(min_size > m_size);
auto capacity = std::max(min_size, m_buffer.size() + std::max(m_buffer.size() / size_type{ 2 }, size_type{ 1 })); // growth factor of 1.5ish
auto buffer = memory_block<value_type>(capacity);
std::uninitialized_copy_n(m_buffer.data(), m_size, buffer.data()); // copy each element from old buffer to new buffer w/ placement new
destruct_all(); // clean up the old buffer (call destructors on each of the old elements)
m_buffer = std::move(buffer); // take ownership of the new buffer
}
template<class Ty>
void vector<Ty>::construct(size_type index, const value_type& value)
{
new (m_buffer.at(index)) value_type(value); // placement new w/ copy constructor
}
template<class Ty>
void vector<Ty>::destruct(size_type index)
{
assert(index < m_size);
m_buffer.at(index)->~value_type(); // explictly call destructor (because we used placement new)
}
template<class Ty>
void vector<Ty>::destruct_all()
{
for (auto index = size_type{ 0 }; index != m_size; ++index)
destruct(index);
}
} // nonstd
int main()
{
{
nonstd::vector<int> v;
v.push_back(10);
}
{
nonstd::vector<int> v(5);
v.pop_back();
}
{
nonstd::vector<int> v(5, 1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(v1);
}
{
nonstd::vector<int> v1(2, 2);
nonstd::vector<int> v2(std::move(v1));
}
}
Note the use of the "copy and swap" idiom in the copy and move assignment operators, which makes implementing them quite a lot easier.
edited 12 hours ago
answered 12 hours ago
user673679user673679
2,5131925
2,5131925
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
add a comment |
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
This is amazing, I learnt a lot from your code, especially the placement new and memory block part. Thanks a lot!!
– Einiemand
7 hours ago
add a comment |
Undefined type
We use size_t
with no definition. It seems like we want to use std::size_t
, in which case, we need a suitable include, such as <cstddef>
.
We also need to include <utility>
for std::move()
and <stdexcept>
for std::exception
.
Missing types
This template accepts only one argument, but a standard vector also has an Allocator
template argument (which defaults to std::allocator<T>
). There's quite a lot that will need to be changed to accept and use a provided allocator class.
std::vector
must provide member types value_type
, allocator_type
, size_type
, difference_type
, reference
, const_reference
, pointer
, const_pointer
, reverse_iterator
and const_reverse_iterator
, but these are all missing from this implementation.
Missing member functions
There's no public assign()
, at()
, data()
, empty()
, max_size()
, reserve()
, shrink_to_fit()
, clear()
, insert()
, emplace()
, erase()
, emplace_back()
, resize()
or swap()
members. Were also missing the const/reverse versions of begin()
and end()
, such as rbegin()
and crend()
.
Constructors and assignment
We're missing the initializer-list versions of the constructor and assignment operator.
The line lengths are very long - consider using a separate line for each initializer, like this:
template<typename Ty>
vector<Ty>::vector()
: buffer(new Ty[10]),
m_first(buffer),
m_last(buffer),
m_end(buffer + 10)
{
}
I'm not sure where the magic number 10
comes from in the above - it's probably worth defining a private constant default_initial_capacity
for this.
The constructor that accepts a single count
argument fails to initialize its elements (std::vector<int>(5)
will create a vector containing five zeros, but our equivalent will have five uninitialized values, which may well cause bugs). This could be avoided by delegating to vector(count, T{})
. We should also check for a size of zero and either avoid allocating, or round up to our default capacity in that case.
The (count, val)
constructor won't compile, as it attempts to modify const count
. We could make count
mutable, but I think we should simply use std::fill()
(from <algorithm>
):
template<typename Ty>
vector<Ty>::vector(const std::size_t count, const Ty& val)
: buffer(new Ty[count]),
m_first(buffer),
m_last(buffer + count),
m_end(buffer + count)
{
std::fill(m_first, m_last, val);
}
Copy constructor could usefully shrink to fit, using the size rather than the capacity of the source vector to determine the new vector's capacity:
template<typename Ty>
vector<Ty>::vector(const vector& other)
: buffer(new Ty[other.size()]),
m_first(buffer),
m_last(buffer + other.size()),
m_end(m_last)
{
std::copy(other.m_first, other.m_last, m_first);
}
Again, I use a standard algorithm to avoid hand-coding my own loop.
Move-construction and move-assignment are most easily implemented using swap()
:
template<typename Ty>
vector<Ty>::vector(vector&& other)
: vector()
{
swap(other);
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other)
{
swap(other);
return *this;
}
It certainly looks wrong to explicitly call the destructor in the assignment operator. All we're using it for is to delete buffer
, so just do that. Better still, use the copy-and-swap idiom:
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other)
{
swap(vector<Ty>(other)); // copy and swap
return *this;
}
Destructor
There's no need to assign nullptr
to the members - they are about to go out of scope, so can't be accessed after the destructor finishes. Also, there's no need to test that buffer
is non-null, for two reasons: first, our logic never allows a non-null buffer to exist, and secondly, delete
will happily do nothing if its argument is null.
Modifiers
Look at push_back()
:
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
See how *(m_last++) = val;
is common to both paths? We can reorder the test so that we don't duplicate that; to my eyes at least, that makes a more natural expression ("ensure there's room, then add the element"):
template<typename Ty>
void vector<Ty>::push_back(const Ty& val)
{
if (capacity() <= size()) {
realloc(2, 2);
}
*(m_last++) = val;
}
Once rbegin()
is implemented, back()
can be re-written to use that rather than doing arithmetic on the result of end()
.
Exceptions
std::exception
has no constructor that accepts a string literal - we need to use a more specific sub-class, such as std::out_of_range
. We should consider whether we have range-checking at all, outside of methods such as at()
which mandate it - standard C++ practice is to impose minimum overhead unless it's asked for.
Private members
Mostly looks okay, though alloc
could be improved using std::move()
algorithm instead of a hand-written copy loop, and simple delete buffer
instead of calling destructor, as described above. It's also wise to allow this function to shrink the buffer; we'll need that for some of the as-yet-unimplemented code:
template<typename Ty>
void vector<Ty>::alloc(const std::size_t cap) {
Ty* new_buffer = new Ty[cap];
auto sz = size();
if (sz < cap) {
sz = cap;
}
std::move(m_first, m_first + sz, new_buffer);
delete buffer;
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
Redundant member
Is there any need for separate buffer
and m_first
members? They have the same type, and as far as I can see, they always hold the same value.
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
that's why they have replaced one ctorvector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11
– RiaD
7 hours ago
You're right @RiaD. I was misremembering, and thought thatvalue_type
needed to beDefaultConstructible
.
– Toby Speight
7 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
add a comment |
Undefined type
We use size_t
with no definition. It seems like we want to use std::size_t
, in which case, we need a suitable include, such as <cstddef>
.
We also need to include <utility>
for std::move()
and <stdexcept>
for std::exception
.
Missing types
This template accepts only one argument, but a standard vector also has an Allocator
template argument (which defaults to std::allocator<T>
). There's quite a lot that will need to be changed to accept and use a provided allocator class.
std::vector
must provide member types value_type
, allocator_type
, size_type
, difference_type
, reference
, const_reference
, pointer
, const_pointer
, reverse_iterator
and const_reverse_iterator
, but these are all missing from this implementation.
Missing member functions
There's no public assign()
, at()
, data()
, empty()
, max_size()
, reserve()
, shrink_to_fit()
, clear()
, insert()
, emplace()
, erase()
, emplace_back()
, resize()
or swap()
members. Were also missing the const/reverse versions of begin()
and end()
, such as rbegin()
and crend()
.
Constructors and assignment
We're missing the initializer-list versions of the constructor and assignment operator.
The line lengths are very long - consider using a separate line for each initializer, like this:
template<typename Ty>
vector<Ty>::vector()
: buffer(new Ty[10]),
m_first(buffer),
m_last(buffer),
m_end(buffer + 10)
{
}
I'm not sure where the magic number 10
comes from in the above - it's probably worth defining a private constant default_initial_capacity
for this.
The constructor that accepts a single count
argument fails to initialize its elements (std::vector<int>(5)
will create a vector containing five zeros, but our equivalent will have five uninitialized values, which may well cause bugs). This could be avoided by delegating to vector(count, T{})
. We should also check for a size of zero and either avoid allocating, or round up to our default capacity in that case.
The (count, val)
constructor won't compile, as it attempts to modify const count
. We could make count
mutable, but I think we should simply use std::fill()
(from <algorithm>
):
template<typename Ty>
vector<Ty>::vector(const std::size_t count, const Ty& val)
: buffer(new Ty[count]),
m_first(buffer),
m_last(buffer + count),
m_end(buffer + count)
{
std::fill(m_first, m_last, val);
}
Copy constructor could usefully shrink to fit, using the size rather than the capacity of the source vector to determine the new vector's capacity:
template<typename Ty>
vector<Ty>::vector(const vector& other)
: buffer(new Ty[other.size()]),
m_first(buffer),
m_last(buffer + other.size()),
m_end(m_last)
{
std::copy(other.m_first, other.m_last, m_first);
}
Again, I use a standard algorithm to avoid hand-coding my own loop.
Move-construction and move-assignment are most easily implemented using swap()
:
template<typename Ty>
vector<Ty>::vector(vector&& other)
: vector()
{
swap(other);
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other)
{
swap(other);
return *this;
}
It certainly looks wrong to explicitly call the destructor in the assignment operator. All we're using it for is to delete buffer
, so just do that. Better still, use the copy-and-swap idiom:
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other)
{
swap(vector<Ty>(other)); // copy and swap
return *this;
}
Destructor
There's no need to assign nullptr
to the members - they are about to go out of scope, so can't be accessed after the destructor finishes. Also, there's no need to test that buffer
is non-null, for two reasons: first, our logic never allows a non-null buffer to exist, and secondly, delete
will happily do nothing if its argument is null.
Modifiers
Look at push_back()
:
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
See how *(m_last++) = val;
is common to both paths? We can reorder the test so that we don't duplicate that; to my eyes at least, that makes a more natural expression ("ensure there's room, then add the element"):
template<typename Ty>
void vector<Ty>::push_back(const Ty& val)
{
if (capacity() <= size()) {
realloc(2, 2);
}
*(m_last++) = val;
}
Once rbegin()
is implemented, back()
can be re-written to use that rather than doing arithmetic on the result of end()
.
Exceptions
std::exception
has no constructor that accepts a string literal - we need to use a more specific sub-class, such as std::out_of_range
. We should consider whether we have range-checking at all, outside of methods such as at()
which mandate it - standard C++ practice is to impose minimum overhead unless it's asked for.
Private members
Mostly looks okay, though alloc
could be improved using std::move()
algorithm instead of a hand-written copy loop, and simple delete buffer
instead of calling destructor, as described above. It's also wise to allow this function to shrink the buffer; we'll need that for some of the as-yet-unimplemented code:
template<typename Ty>
void vector<Ty>::alloc(const std::size_t cap) {
Ty* new_buffer = new Ty[cap];
auto sz = size();
if (sz < cap) {
sz = cap;
}
std::move(m_first, m_first + sz, new_buffer);
delete buffer;
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
Redundant member
Is there any need for separate buffer
and m_first
members? They have the same type, and as far as I can see, they always hold the same value.
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
that's why they have replaced one ctorvector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11
– RiaD
7 hours ago
You're right @RiaD. I was misremembering, and thought thatvalue_type
needed to beDefaultConstructible
.
– Toby Speight
7 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
add a comment |
Undefined type
We use size_t
with no definition. It seems like we want to use std::size_t
, in which case, we need a suitable include, such as <cstddef>
.
We also need to include <utility>
for std::move()
and <stdexcept>
for std::exception
.
Missing types
This template accepts only one argument, but a standard vector also has an Allocator
template argument (which defaults to std::allocator<T>
). There's quite a lot that will need to be changed to accept and use a provided allocator class.
std::vector
must provide member types value_type
, allocator_type
, size_type
, difference_type
, reference
, const_reference
, pointer
, const_pointer
, reverse_iterator
and const_reverse_iterator
, but these are all missing from this implementation.
Missing member functions
There's no public assign()
, at()
, data()
, empty()
, max_size()
, reserve()
, shrink_to_fit()
, clear()
, insert()
, emplace()
, erase()
, emplace_back()
, resize()
or swap()
members. Were also missing the const/reverse versions of begin()
and end()
, such as rbegin()
and crend()
.
Constructors and assignment
We're missing the initializer-list versions of the constructor and assignment operator.
The line lengths are very long - consider using a separate line for each initializer, like this:
template<typename Ty>
vector<Ty>::vector()
: buffer(new Ty[10]),
m_first(buffer),
m_last(buffer),
m_end(buffer + 10)
{
}
I'm not sure where the magic number 10
comes from in the above - it's probably worth defining a private constant default_initial_capacity
for this.
The constructor that accepts a single count
argument fails to initialize its elements (std::vector<int>(5)
will create a vector containing five zeros, but our equivalent will have five uninitialized values, which may well cause bugs). This could be avoided by delegating to vector(count, T{})
. We should also check for a size of zero and either avoid allocating, or round up to our default capacity in that case.
The (count, val)
constructor won't compile, as it attempts to modify const count
. We could make count
mutable, but I think we should simply use std::fill()
(from <algorithm>
):
template<typename Ty>
vector<Ty>::vector(const std::size_t count, const Ty& val)
: buffer(new Ty[count]),
m_first(buffer),
m_last(buffer + count),
m_end(buffer + count)
{
std::fill(m_first, m_last, val);
}
Copy constructor could usefully shrink to fit, using the size rather than the capacity of the source vector to determine the new vector's capacity:
template<typename Ty>
vector<Ty>::vector(const vector& other)
: buffer(new Ty[other.size()]),
m_first(buffer),
m_last(buffer + other.size()),
m_end(m_last)
{
std::copy(other.m_first, other.m_last, m_first);
}
Again, I use a standard algorithm to avoid hand-coding my own loop.
Move-construction and move-assignment are most easily implemented using swap()
:
template<typename Ty>
vector<Ty>::vector(vector&& other)
: vector()
{
swap(other);
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other)
{
swap(other);
return *this;
}
It certainly looks wrong to explicitly call the destructor in the assignment operator. All we're using it for is to delete buffer
, so just do that. Better still, use the copy-and-swap idiom:
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other)
{
swap(vector<Ty>(other)); // copy and swap
return *this;
}
Destructor
There's no need to assign nullptr
to the members - they are about to go out of scope, so can't be accessed after the destructor finishes. Also, there's no need to test that buffer
is non-null, for two reasons: first, our logic never allows a non-null buffer to exist, and secondly, delete
will happily do nothing if its argument is null.
Modifiers
Look at push_back()
:
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
See how *(m_last++) = val;
is common to both paths? We can reorder the test so that we don't duplicate that; to my eyes at least, that makes a more natural expression ("ensure there's room, then add the element"):
template<typename Ty>
void vector<Ty>::push_back(const Ty& val)
{
if (capacity() <= size()) {
realloc(2, 2);
}
*(m_last++) = val;
}
Once rbegin()
is implemented, back()
can be re-written to use that rather than doing arithmetic on the result of end()
.
Exceptions
std::exception
has no constructor that accepts a string literal - we need to use a more specific sub-class, such as std::out_of_range
. We should consider whether we have range-checking at all, outside of methods such as at()
which mandate it - standard C++ practice is to impose minimum overhead unless it's asked for.
Private members
Mostly looks okay, though alloc
could be improved using std::move()
algorithm instead of a hand-written copy loop, and simple delete buffer
instead of calling destructor, as described above. It's also wise to allow this function to shrink the buffer; we'll need that for some of the as-yet-unimplemented code:
template<typename Ty>
void vector<Ty>::alloc(const std::size_t cap) {
Ty* new_buffer = new Ty[cap];
auto sz = size();
if (sz < cap) {
sz = cap;
}
std::move(m_first, m_first + sz, new_buffer);
delete buffer;
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
Redundant member
Is there any need for separate buffer
and m_first
members? They have the same type, and as far as I can see, they always hold the same value.
Undefined type
We use size_t
with no definition. It seems like we want to use std::size_t
, in which case, we need a suitable include, such as <cstddef>
.
We also need to include <utility>
for std::move()
and <stdexcept>
for std::exception
.
Missing types
This template accepts only one argument, but a standard vector also has an Allocator
template argument (which defaults to std::allocator<T>
). There's quite a lot that will need to be changed to accept and use a provided allocator class.
std::vector
must provide member types value_type
, allocator_type
, size_type
, difference_type
, reference
, const_reference
, pointer
, const_pointer
, reverse_iterator
and const_reverse_iterator
, but these are all missing from this implementation.
Missing member functions
There's no public assign()
, at()
, data()
, empty()
, max_size()
, reserve()
, shrink_to_fit()
, clear()
, insert()
, emplace()
, erase()
, emplace_back()
, resize()
or swap()
members. Were also missing the const/reverse versions of begin()
and end()
, such as rbegin()
and crend()
.
Constructors and assignment
We're missing the initializer-list versions of the constructor and assignment operator.
The line lengths are very long - consider using a separate line for each initializer, like this:
template<typename Ty>
vector<Ty>::vector()
: buffer(new Ty[10]),
m_first(buffer),
m_last(buffer),
m_end(buffer + 10)
{
}
I'm not sure where the magic number 10
comes from in the above - it's probably worth defining a private constant default_initial_capacity
for this.
The constructor that accepts a single count
argument fails to initialize its elements (std::vector<int>(5)
will create a vector containing five zeros, but our equivalent will have five uninitialized values, which may well cause bugs). This could be avoided by delegating to vector(count, T{})
. We should also check for a size of zero and either avoid allocating, or round up to our default capacity in that case.
The (count, val)
constructor won't compile, as it attempts to modify const count
. We could make count
mutable, but I think we should simply use std::fill()
(from <algorithm>
):
template<typename Ty>
vector<Ty>::vector(const std::size_t count, const Ty& val)
: buffer(new Ty[count]),
m_first(buffer),
m_last(buffer + count),
m_end(buffer + count)
{
std::fill(m_first, m_last, val);
}
Copy constructor could usefully shrink to fit, using the size rather than the capacity of the source vector to determine the new vector's capacity:
template<typename Ty>
vector<Ty>::vector(const vector& other)
: buffer(new Ty[other.size()]),
m_first(buffer),
m_last(buffer + other.size()),
m_end(m_last)
{
std::copy(other.m_first, other.m_last, m_first);
}
Again, I use a standard algorithm to avoid hand-coding my own loop.
Move-construction and move-assignment are most easily implemented using swap()
:
template<typename Ty>
vector<Ty>::vector(vector&& other)
: vector()
{
swap(other);
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other)
{
swap(other);
return *this;
}
It certainly looks wrong to explicitly call the destructor in the assignment operator. All we're using it for is to delete buffer
, so just do that. Better still, use the copy-and-swap idiom:
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other)
{
swap(vector<Ty>(other)); // copy and swap
return *this;
}
Destructor
There's no need to assign nullptr
to the members - they are about to go out of scope, so can't be accessed after the destructor finishes. Also, there's no need to test that buffer
is non-null, for two reasons: first, our logic never allows a non-null buffer to exist, and secondly, delete
will happily do nothing if its argument is null.
Modifiers
Look at push_back()
:
template<typename Ty>
void vector<Ty>::push_back(const Ty& val) {
if (size() < capacity()) {
*(m_last++) = val;
return;
}
realloc(2, 2);
*(m_last++) = val;
}
See how *(m_last++) = val;
is common to both paths? We can reorder the test so that we don't duplicate that; to my eyes at least, that makes a more natural expression ("ensure there's room, then add the element"):
template<typename Ty>
void vector<Ty>::push_back(const Ty& val)
{
if (capacity() <= size()) {
realloc(2, 2);
}
*(m_last++) = val;
}
Once rbegin()
is implemented, back()
can be re-written to use that rather than doing arithmetic on the result of end()
.
Exceptions
std::exception
has no constructor that accepts a string literal - we need to use a more specific sub-class, such as std::out_of_range
. We should consider whether we have range-checking at all, outside of methods such as at()
which mandate it - standard C++ practice is to impose minimum overhead unless it's asked for.
Private members
Mostly looks okay, though alloc
could be improved using std::move()
algorithm instead of a hand-written copy loop, and simple delete buffer
instead of calling destructor, as described above. It's also wise to allow this function to shrink the buffer; we'll need that for some of the as-yet-unimplemented code:
template<typename Ty>
void vector<Ty>::alloc(const std::size_t cap) {
Ty* new_buffer = new Ty[cap];
auto sz = size();
if (sz < cap) {
sz = cap;
}
std::move(m_first, m_first + sz, new_buffer);
delete buffer;
buffer = new_buffer;
m_first = buffer;
m_last = buffer + sz;
m_end = buffer + cap;
}
Redundant member
Is there any need for separate buffer
and m_first
members? They have the same type, and as far as I can see, they always hold the same value.
answered 15 hours ago


Toby SpeightToby Speight
23.5k639111
23.5k639111
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
that's why they have replaced one ctorvector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11
– RiaD
7 hours ago
You're right @RiaD. I was misremembering, and thought thatvalue_type
needed to beDefaultConstructible
.
– Toby Speight
7 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
add a comment |
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
that's why they have replaced one ctorvector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11
– RiaD
7 hours ago
You're right @RiaD. I was misremembering, and thought thatvalue_type
needed to beDefaultConstructible
.
– Toby Speight
7 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
> This could be avoided by delegating to vector(count, T{}) you can't delegate this way in general case, in case there's no copy constructor and/or default ctor does something unusual e.g. fills its element with random value (and on copy values is copied)
– RiaD
7 hours ago
that's why they have replaced one ctor
vector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11– RiaD
7 hours ago
that's why they have replaced one ctor
vector (size_type n, const value_type& val = value_type()
with two separate ctors since c++11– RiaD
7 hours ago
You're right @RiaD. I was misremembering, and thought that
value_type
needed to be DefaultConstructible
.– Toby Speight
7 hours ago
You're right @RiaD. I was misremembering, and thought that
value_type
needed to be DefaultConstructible
.– Toby Speight
7 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
@Incomputable, sorry, but I don't understand how copy elision is relevant at all
– RiaD
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
I apologize for my negligence. Somehow I thought the story is only about one object being constructed. Perhaps I dismissed the rest of the problem too quickly.
– Incomputable
6 hours ago
add a comment |
- Missing includes:
- You need an include for
std::size_t
(though you could just usesize_type
everywhere, and declare that asusing size_type = decltype(sizeof 0);
. - You use
std::move
, and thus need<utility>
.
- You need an include for
namespace nonstd {
template<typename Ty>
class vector
If (nearly) everything is in the same namespace, it is conventional not to indent it.
Obviously, you forego allocator-support (for now?).
Using
Ty
instead ofT
is very unconventional. Following convention reduces cognitive load, allowing you to expend it elsewhere more profitably. I'll pretend you followed convention for the rest of the review.
using iterator = Ty * ;
using const_iterator = const Ty*;
The whitespace around the first
*
is misplaced. Consider auto-formatting, or just be a bit more careful to stay consistent.Those aliases are good. You are missing myriad other useful (and necessary) ones though, namely
value_type
,size_type
,difference_type
,reference
,const_reference
,pointer
, andconst_pointer
.
And when you add reverse-iterator-support,reverse_iterator
andconst_reverse_iterator
.
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
There is no reason not to make the default-ctor
noexcept
andconstexpr
, even if that means not allocating any memory. Especially as passing a zero count to a ctor or invoking the move-ctor can already result in a 0-capacity vector.
Also, the magic constant you chose is pretty arbitrary, and should be a (private) named constant if you decide to keep it.Having
const
parameters in an API is at best irritating to the reader: It doesn't actually do anything.You are missing constructors from
std::initializer_list<T>
and iterator-pair. At least copy-ctor and creating from initializer-list should then delegate to iterator-pair for code-reuse.The move-ctor cannot throw by design, and should thus be marked
noexcept
.Dito for move-assignment.
size_t size() const;
size_t capacity() const;
Both observers above could be
constexpr
if you have at least oneconstexpr
ctor...You are missing
empty()
, andmax_size()
.
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
You are missing
emplace_back()
, which the first two should delegate to.Also missing are
clear()
,insert()
,emplace()
,erase()
, andswap()
. The last one is crucial for a simple, efficient and exception-safe implementation of much of the class.
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
All the mutable versions can be implemented by delegating to the
const
variant and applying a judiciousconst_cast
to the result.
Especially as you decided that "undefined behavior" means "throws exception" in your case.Even though you gave the above observers the behavior of
at()
, you shouldn't leave it out.You are missing
data()
completely. Yes, it behaves the same asbegin()
for you, but there you have it.
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
- You are missing the reverse_iterator equivalents.
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
- Either
buffer
orm_first
is redundant.
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
Centralizing allocation to enforce policy and reduce repetition is a good idea. You aren't quite there though, and implementing the wrong signature. You need one private member for automatic growth
void grow(size_type n = 1)
and the rest should be public, so a consumer who knows can use it:
void reserve(size_type n);
void resize(size_type n);
void shrink_to_fit();
Your current allocation-policy is "double plus constant" on reallocation. That's actually the worst one you could choose, as it makes it impossible to reuse any memory over multiple consecutive reallocations (all the returned memory from earlier together is never enough). For that, it should be below two, maybe
m_new = max(needed, m_old * 7 / 4)
for automatic reallocation.You are missing assignment from
std::initializer_list<T>
, the more versatileassign()
,
Now only implementation-details:
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
// and many more
- You are aware that
new T[n]
doesn't only reserve space forn
T
s, but also constructs them? That extra-work might be quite involved, and is most emphatically not expected. You should only allocate raw memory (by calling e. g.void* operator new(std::size_t)
) and construct the members on demand as needed.
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
- Why do you guard against deleting a nullpointer? Also, why repaint the house when demolishing it?
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
Don't pessimise the common case, by optimising self-assignment.
Calling the dtor means the lifetime ended. Pretending it didn't causes Undefined Behavior.
Assignment should be transactional: Either succeed, or have no effect. Look into the copy-and-swap idiom.
Move-assignment should be simple and efficient: Just
swap()
.
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
pop_back()
seems to be the only part ofvector
assuming you construct and destruct elements on demand. That mismatch can be explosive.
add a comment |
- Missing includes:
- You need an include for
std::size_t
(though you could just usesize_type
everywhere, and declare that asusing size_type = decltype(sizeof 0);
. - You use
std::move
, and thus need<utility>
.
- You need an include for
namespace nonstd {
template<typename Ty>
class vector
If (nearly) everything is in the same namespace, it is conventional not to indent it.
Obviously, you forego allocator-support (for now?).
Using
Ty
instead ofT
is very unconventional. Following convention reduces cognitive load, allowing you to expend it elsewhere more profitably. I'll pretend you followed convention for the rest of the review.
using iterator = Ty * ;
using const_iterator = const Ty*;
The whitespace around the first
*
is misplaced. Consider auto-formatting, or just be a bit more careful to stay consistent.Those aliases are good. You are missing myriad other useful (and necessary) ones though, namely
value_type
,size_type
,difference_type
,reference
,const_reference
,pointer
, andconst_pointer
.
And when you add reverse-iterator-support,reverse_iterator
andconst_reverse_iterator
.
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
There is no reason not to make the default-ctor
noexcept
andconstexpr
, even if that means not allocating any memory. Especially as passing a zero count to a ctor or invoking the move-ctor can already result in a 0-capacity vector.
Also, the magic constant you chose is pretty arbitrary, and should be a (private) named constant if you decide to keep it.Having
const
parameters in an API is at best irritating to the reader: It doesn't actually do anything.You are missing constructors from
std::initializer_list<T>
and iterator-pair. At least copy-ctor and creating from initializer-list should then delegate to iterator-pair for code-reuse.The move-ctor cannot throw by design, and should thus be marked
noexcept
.Dito for move-assignment.
size_t size() const;
size_t capacity() const;
Both observers above could be
constexpr
if you have at least oneconstexpr
ctor...You are missing
empty()
, andmax_size()
.
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
You are missing
emplace_back()
, which the first two should delegate to.Also missing are
clear()
,insert()
,emplace()
,erase()
, andswap()
. The last one is crucial for a simple, efficient and exception-safe implementation of much of the class.
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
All the mutable versions can be implemented by delegating to the
const
variant and applying a judiciousconst_cast
to the result.
Especially as you decided that "undefined behavior" means "throws exception" in your case.Even though you gave the above observers the behavior of
at()
, you shouldn't leave it out.You are missing
data()
completely. Yes, it behaves the same asbegin()
for you, but there you have it.
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
- You are missing the reverse_iterator equivalents.
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
- Either
buffer
orm_first
is redundant.
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
Centralizing allocation to enforce policy and reduce repetition is a good idea. You aren't quite there though, and implementing the wrong signature. You need one private member for automatic growth
void grow(size_type n = 1)
and the rest should be public, so a consumer who knows can use it:
void reserve(size_type n);
void resize(size_type n);
void shrink_to_fit();
Your current allocation-policy is "double plus constant" on reallocation. That's actually the worst one you could choose, as it makes it impossible to reuse any memory over multiple consecutive reallocations (all the returned memory from earlier together is never enough). For that, it should be below two, maybe
m_new = max(needed, m_old * 7 / 4)
for automatic reallocation.You are missing assignment from
std::initializer_list<T>
, the more versatileassign()
,
Now only implementation-details:
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
// and many more
- You are aware that
new T[n]
doesn't only reserve space forn
T
s, but also constructs them? That extra-work might be quite involved, and is most emphatically not expected. You should only allocate raw memory (by calling e. g.void* operator new(std::size_t)
) and construct the members on demand as needed.
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
- Why do you guard against deleting a nullpointer? Also, why repaint the house when demolishing it?
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
Don't pessimise the common case, by optimising self-assignment.
Calling the dtor means the lifetime ended. Pretending it didn't causes Undefined Behavior.
Assignment should be transactional: Either succeed, or have no effect. Look into the copy-and-swap idiom.
Move-assignment should be simple and efficient: Just
swap()
.
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
pop_back()
seems to be the only part ofvector
assuming you construct and destruct elements on demand. That mismatch can be explosive.
add a comment |
- Missing includes:
- You need an include for
std::size_t
(though you could just usesize_type
everywhere, and declare that asusing size_type = decltype(sizeof 0);
. - You use
std::move
, and thus need<utility>
.
- You need an include for
namespace nonstd {
template<typename Ty>
class vector
If (nearly) everything is in the same namespace, it is conventional not to indent it.
Obviously, you forego allocator-support (for now?).
Using
Ty
instead ofT
is very unconventional. Following convention reduces cognitive load, allowing you to expend it elsewhere more profitably. I'll pretend you followed convention for the rest of the review.
using iterator = Ty * ;
using const_iterator = const Ty*;
The whitespace around the first
*
is misplaced. Consider auto-formatting, or just be a bit more careful to stay consistent.Those aliases are good. You are missing myriad other useful (and necessary) ones though, namely
value_type
,size_type
,difference_type
,reference
,const_reference
,pointer
, andconst_pointer
.
And when you add reverse-iterator-support,reverse_iterator
andconst_reverse_iterator
.
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
There is no reason not to make the default-ctor
noexcept
andconstexpr
, even if that means not allocating any memory. Especially as passing a zero count to a ctor or invoking the move-ctor can already result in a 0-capacity vector.
Also, the magic constant you chose is pretty arbitrary, and should be a (private) named constant if you decide to keep it.Having
const
parameters in an API is at best irritating to the reader: It doesn't actually do anything.You are missing constructors from
std::initializer_list<T>
and iterator-pair. At least copy-ctor and creating from initializer-list should then delegate to iterator-pair for code-reuse.The move-ctor cannot throw by design, and should thus be marked
noexcept
.Dito for move-assignment.
size_t size() const;
size_t capacity() const;
Both observers above could be
constexpr
if you have at least oneconstexpr
ctor...You are missing
empty()
, andmax_size()
.
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
You are missing
emplace_back()
, which the first two should delegate to.Also missing are
clear()
,insert()
,emplace()
,erase()
, andswap()
. The last one is crucial for a simple, efficient and exception-safe implementation of much of the class.
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
All the mutable versions can be implemented by delegating to the
const
variant and applying a judiciousconst_cast
to the result.
Especially as you decided that "undefined behavior" means "throws exception" in your case.Even though you gave the above observers the behavior of
at()
, you shouldn't leave it out.You are missing
data()
completely. Yes, it behaves the same asbegin()
for you, but there you have it.
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
- You are missing the reverse_iterator equivalents.
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
- Either
buffer
orm_first
is redundant.
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
Centralizing allocation to enforce policy and reduce repetition is a good idea. You aren't quite there though, and implementing the wrong signature. You need one private member for automatic growth
void grow(size_type n = 1)
and the rest should be public, so a consumer who knows can use it:
void reserve(size_type n);
void resize(size_type n);
void shrink_to_fit();
Your current allocation-policy is "double plus constant" on reallocation. That's actually the worst one you could choose, as it makes it impossible to reuse any memory over multiple consecutive reallocations (all the returned memory from earlier together is never enough). For that, it should be below two, maybe
m_new = max(needed, m_old * 7 / 4)
for automatic reallocation.You are missing assignment from
std::initializer_list<T>
, the more versatileassign()
,
Now only implementation-details:
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
// and many more
- You are aware that
new T[n]
doesn't only reserve space forn
T
s, but also constructs them? That extra-work might be quite involved, and is most emphatically not expected. You should only allocate raw memory (by calling e. g.void* operator new(std::size_t)
) and construct the members on demand as needed.
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
- Why do you guard against deleting a nullpointer? Also, why repaint the house when demolishing it?
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
Don't pessimise the common case, by optimising self-assignment.
Calling the dtor means the lifetime ended. Pretending it didn't causes Undefined Behavior.
Assignment should be transactional: Either succeed, or have no effect. Look into the copy-and-swap idiom.
Move-assignment should be simple and efficient: Just
swap()
.
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
pop_back()
seems to be the only part ofvector
assuming you construct and destruct elements on demand. That mismatch can be explosive.
- Missing includes:
- You need an include for
std::size_t
(though you could just usesize_type
everywhere, and declare that asusing size_type = decltype(sizeof 0);
. - You use
std::move
, and thus need<utility>
.
- You need an include for
namespace nonstd {
template<typename Ty>
class vector
If (nearly) everything is in the same namespace, it is conventional not to indent it.
Obviously, you forego allocator-support (for now?).
Using
Ty
instead ofT
is very unconventional. Following convention reduces cognitive load, allowing you to expend it elsewhere more profitably. I'll pretend you followed convention for the rest of the review.
using iterator = Ty * ;
using const_iterator = const Ty*;
The whitespace around the first
*
is misplaced. Consider auto-formatting, or just be a bit more careful to stay consistent.Those aliases are good. You are missing myriad other useful (and necessary) ones though, namely
value_type
,size_type
,difference_type
,reference
,const_reference
,pointer
, andconst_pointer
.
And when you add reverse-iterator-support,reverse_iterator
andconst_reverse_iterator
.
vector();
explicit vector(const size_t count);
vector(const size_t count, const Ty& val);
vector(const vector& other);
vector(vector&& other);
~vector();
vector& operator=(const vector& other);
vector& operator=(vector&& other);
There is no reason not to make the default-ctor
noexcept
andconstexpr
, even if that means not allocating any memory. Especially as passing a zero count to a ctor or invoking the move-ctor can already result in a 0-capacity vector.
Also, the magic constant you chose is pretty arbitrary, and should be a (private) named constant if you decide to keep it.Having
const
parameters in an API is at best irritating to the reader: It doesn't actually do anything.You are missing constructors from
std::initializer_list<T>
and iterator-pair. At least copy-ctor and creating from initializer-list should then delegate to iterator-pair for code-reuse.The move-ctor cannot throw by design, and should thus be marked
noexcept
.Dito for move-assignment.
size_t size() const;
size_t capacity() const;
Both observers above could be
constexpr
if you have at least oneconstexpr
ctor...You are missing
empty()
, andmax_size()
.
void push_back(const Ty& val);
void push_back(Ty&& val);
void pop_back();
You are missing
emplace_back()
, which the first two should delegate to.Also missing are
clear()
,insert()
,emplace()
,erase()
, andswap()
. The last one is crucial for a simple, efficient and exception-safe implementation of much of the class.
Ty& front();
const Ty& front() const;
Ty& back();
const Ty& back() const;
Ty& operator(const size_t pos);
const Ty& operator(const size_t pos) const;
All the mutable versions can be implemented by delegating to the
const
variant and applying a judiciousconst_cast
to the result.
Especially as you decided that "undefined behavior" means "throws exception" in your case.Even though you gave the above observers the behavior of
at()
, you shouldn't leave it out.You are missing
data()
completely. Yes, it behaves the same asbegin()
for you, but there you have it.
iterator begin();
const_iterator begin() const;
iterator end();
const_iterator end() const;
- You are missing the reverse_iterator equivalents.
Ty * buffer;
iterator m_first;
iterator m_last;
iterator m_end;
- Either
buffer
orm_first
is redundant.
void realloc(const size_t factor, const size_t carry);
void alloc(const size_t cap);
Centralizing allocation to enforce policy and reduce repetition is a good idea. You aren't quite there though, and implementing the wrong signature. You need one private member for automatic growth
void grow(size_type n = 1)
and the rest should be public, so a consumer who knows can use it:
void reserve(size_type n);
void resize(size_type n);
void shrink_to_fit();
Your current allocation-policy is "double plus constant" on reallocation. That's actually the worst one you could choose, as it makes it impossible to reuse any memory over multiple consecutive reallocations (all the returned memory from earlier together is never enough). For that, it should be below two, maybe
m_new = max(needed, m_old * 7 / 4)
for automatic reallocation.You are missing assignment from
std::initializer_list<T>
, the more versatileassign()
,
Now only implementation-details:
vector<Ty>::vector() : buffer(new Ty[10]), m_first(buffer), m_last(buffer), m_end(buffer + 10) {
// and many more
- You are aware that
new T[n]
doesn't only reserve space forn
T
s, but also constructs them? That extra-work might be quite involved, and is most emphatically not expected. You should only allocate raw memory (by calling e. g.void* operator new(std::size_t)
) and construct the members on demand as needed.
template<typename Ty>
vector<Ty>::~vector() {
if (buffer != nullptr) {
m_first = m_last = m_end = nullptr;
delete buffer;
}
}
- Why do you guard against deleting a nullpointer? Also, why repaint the house when demolishing it?
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(const vector<Ty>& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = new Ty[other.capacity()];
m_first = buffer;
m_last = buffer + other.size();
m_end = buffer + other.capacity();
for (size_t i = 0; i < size(); ++i) {
buffer[i] = other[i];
}
return *this;
}
template<typename Ty>
vector<Ty>& vector<Ty>::operator=(vector<Ty>&& other) {
if (this == &other) {
return *this;
}
this->~vector();
buffer = other.buffer;
m_first = other.m_first;
m_last = other.m_last;
m_end = other.m_end;
other.buffer = nullptr;
other.m_first = other.m_last = other.m_end = nullptr;
return *this;
}
Don't pessimise the common case, by optimising self-assignment.
Calling the dtor means the lifetime ended. Pretending it didn't causes Undefined Behavior.
Assignment should be transactional: Either succeed, or have no effect. Look into the copy-and-swap idiom.
Move-assignment should be simple and efficient: Just
swap()
.
template<typename Ty>
void vector<Ty>::pop_back() {
if (size() == 0) {
throw std::exception("vector is empty");
}
(--m_last)->~Ty();
}
pop_back()
seems to be the only part ofvector
assuming you construct and destruct elements on demand. That mismatch can be explosive.
edited 5 hours ago
answered 11 hours ago
DeduplicatorDeduplicator
11.1k1849
11.1k1849
add a comment |
add a comment |
Einiemand is a new contributor. Be nice, and check out our Code of Conduct.
Einiemand is a new contributor. Be nice, and check out our Code of Conduct.
Einiemand is a new contributor. Be nice, and check out our Code of Conduct.
Einiemand is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211241%2fsimple-re-implementation-of-stdvector%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
VSR qI TU,QBdGAoLKIiw,86twJ VY8LaJs nW,Z4xK u3BWZbRE
You should take a look at the articles I wrote about building your own vector: lokiastari.com/series The main improvement I think you need to make is
Lazy Construction of elements
which requires you to use placement new. See: lokiastari.com/blog/2016/02/27/vector/index.html– Martin York
4 hours ago
Note: If you manually run the destructor of the object
this->~vector();
then the object is no longer valid until you re-run the constructor. Since you are inside the object you would need to call the constructor via placement new.– Martin York
4 hours ago
@MartinYork Thank you, your articles are awesome! Will try again and post it here.
– Einiemand
3 hours ago