C array wrapper

Question

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 ¹, 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 <typename Ty>
class Simple_Array
{
 size_t _size;
 Ty* _data;
public:
 Simple_Array(size_t size) : _size(size), _data(new Ty[size]) {}
 template <class It>
 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;
 }
 virtual ~Simple_Array() {
 if (_data != nullptr)
 delete[] _data;
 }
 const size_t size() const { return _size; }
 Ty* begin() const { return _data; }
 Ty* end() const { return _data + _size; }
 const Ty* cbegin() const { return _data; }
 const Ty* cend() const { return _data + _size; }
 const Ty* at(size_t index) const { return _data + index; }
 Ty& get(size_t index) { return _data[index]; }
 const Ty get(size_t index) const { return _data[index]; }
 Ty& operator[](size_t index) { return _data[index]; }
 const Ty operator[](size_t index) const { return _data[index]; }
private:
 template <class It>
 unchecked_copy(It first, It last) {
#ifdef _SECURE_SCL // MSVC requires checked iterators or disabled warnings.
 std::copy(first, last, stdext::make_unchecked_array_iterator(_data));
#else
 std::copy(first, last, _data);
#endif
 }
};

I haven't tested this on anything other than MSVC, and I'd appreciate any criticism on portability.

¹ Resizing can occur during copy / move assignment, but I'm ok with that.

• ratchet freak: leak in move assign, this->_data doesn't get freed
• AlexeiBarnes: @ratchetfreak Woops, that should have been obvious! Oh well, I updated it with the fix, thanks!
• Deduplicator: Please see: meta.codereview.stackexchange.com/questions/1763/…
• AlexeiBarnes: @Deduplicator Ahh, makes sense, thanks for the pointer, I'll post an answer to my own question with the new code.

Answer 1

Construction

In your constructor from iterators:

template <class It>
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 <class It>
Simple_Array(It first, It last)
: _size(std::distance(first, last))
{
 _data = static_cast<Ty*>(::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<T*>(::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<Ty> 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.

• Jamal: Comments are not for extended discussion; this conversation has been moved to chat.

Answer 2

Everything Barry said.

In addition there are a couple of types I would add.

template <typename Ty>
class Simple_Array
{
 public:
 typedef Ty value_type;
 typedef Ty& reference;
 typedef Ty* pointer;
 typedef Ty const& const_reference;
 typedef Ty const* const_pointer;
 typedef std::size_t size_type;
 typedef std::ptrdiff_t difference_type;
 typedef Ty* iterator;
 typedef Ty const* const_iterator;

Modify your standard methods to use these types. This allows the standard library to produce optimal code when using algorithms.

• AlexeiBarnes: OK, didn't know stl algorithms would take advantage of that but it makes sense, thanks!
• AlexeiBarnes: Presumably by T* iterator and T const* const_iterator you mean Ty.... Also By std::difference_t do you mean std::ptrdiff_t? Can't find anything about difference_t.
• Loki Astari: @AlexeiBarnes: Yes on all counts.
• AlexeiBarnes: Awesome, once I have figured out some things about the copy swap idiom and SFINAE I'll post the revised code including this.
• Loki Astari: @AlexeiBarnes: Just do it in a new question.