C array wrapper

As part of a larger project I wanted to write my own size once RAII C array wrapper. Now I could have just used std::vector, which I'm sure is the first thing almost everyone will want to point out. However, I'm doing this for learning purposes. Aside from wanting to learn more, Simple_Array is not exactly designed to replicate what vector does: it should not resize 1, it should be safe to inherit from.

I'm concerned with both style and efficacy so aside from any and all criticism, I'd love to know if I'm using std::move appropriately. Since it is just being used on _size and _data presumably this changes nothing as they are built in types, is it good etiquette to use std::move anyway?

```
template
class Simple_Array
{
size_t _size;
Ty* _data;

public:
Simple_Array(size_t size) : _size(size), _data(new Ty[size]) {}

template
Simple_Array(It first, It last) :
Simple_Array(std::distance(first, last))
{
unchecked_copy(first, last);
}

Simple_Array(const Simple_Array& other) :
Simple_Array(other.begin(), other.end()) {}

Simple_Array(Simple_Array&& other) :
_size(std::move(other._size)), _data(std::move(other._data))
{
other._size = 0;
other._data = nullptr;
}

Simple_Array& operator=(const Simple_Array& other) {
if (this == &other)
return *this;

if (_size != other._size) {
if (_data != nullptr)
delete[] _data;

_size = other._size;
_data = new Ty[_size];
}

unchecked_copy(other.begin(), other.end());

return *this;
}

Simple_Array& operator=(Simple_Array&& other) {
if (this == &other)
return *this;

if (_data != nullptr)
delete[] _data;

_size = std::move(other._size);
_data = std::move(other._data);

other._size = 0;
other._data = nullptr;

return *this;
}

vir

Construction

In your constructor from iterators:

template 
Simple_Array(It first, It last) :
    Simple_Array(std::distance(first, last))
{
    unchecked_copy(first, last);
}

You do two things: first you default-construct a n objects, and then you copy-assign them. This is both inefficient and reduces the usability of your class. What if Ty is not default constructible? Now I can't have a Simple_Array of it!

Prefer to use the global operator new to just give you uninitialized data, and then placement new into each slot:

template 
Simple_Array(It first, It last)
: _size(std::distance(first, last))
{
    _data = static_cast(::operator new(_size * sizeof(Ty)));

    Ty* cur = _data;
    for (; first != last; ++first, ++cur) {
        new (cur) T{*first};
    }
}

You may also want to add SFINAE to make sure that this constructor is only viable if Ty is constructible from *It.

Note that we have to initialize _data the same way everywhere, so we'll need to change the other constructor too:

SimpleArray(size_t size) : _size(size)
{   
    _data = static_cast(::operator new(_size * sizeof(T)));
    for (Ty* cur = _data; cur != _data + _size; ++cur) {
        new (cur) Ty();
    }
}

Initializer List constructor

Would be helpful to add an initializer_list constructor. Especially since you already have a two-iterator one:

Simple_Array(std::initializer_list elems)
: Simple_Array(elems.begin(), elems.end())
{ }

Copy/Move Assignment

The best way to write copy assignment is copy-and-swap, and the best way to write move assignment is swap. Self-assignment is a pessimization, since that's going to be a rare occurrence anyway. Plus, with swap, we can make everything noexcept:

Simple_Array& operator=(Simple_Array rhs) noexcept {  // copy/move
    swap(rhs);                                        // and swap
    return *this;
}

void swap(Simple_Array& rhs) noexcept {
    std::swap(_size, rhs._size);
    std::swap(_data, rhs._data);
}

As a possible optimization, the copy assignment can reuse the same buffer if it's already large enough and copy assignment for Ty cannot throw (simply then manually destroy all the leftover elements).

Virtual destructor??

There is no reason for the destructor to be virtual. You're adding a vtable to your class for no reason. With the new change to using operator new to allocate the array, we now need to use operator delete to delete it (along with manually destroying every element):

~Simple_Array() {
    for (T* cur = begin(); cur != end(); ++cur) {
        cur->~Ty();
    }

    ::operator delete(_data);
}

const methods should still return references

Your operator[] and get() for non-const return Ty& but for const return const Ty. Why make the extra copy? You should give back a const Ty&.

at() should bounds check

By convention, at() should (a) check that the index is within bounds and throw std::out_of_range() if it's not and (b) return a reference. That's what std::vector, std::deque, std::string, std::array, etc. all do.

StackExchange Code Review Q#113487, answer score: 5