5
\$\begingroup\$

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 <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.

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

\$\endgroup\$
  • \$\begingroup\$ leak in move assign, this->_data doesn't get freed \$\endgroup\$ – ratchet freak Dec 10 '15 at 15:27
  • \$\begingroup\$ @ratchetfreak Woops, that should have been obvious! Oh well, I updated it with the fix, thanks! \$\endgroup\$ – AlexeiBarnes Dec 10 '15 at 15:30
  • 1
    \$\begingroup\$ Please see: meta.codereview.stackexchange.com/questions/1763/… \$\endgroup\$ – Deduplicator Dec 10 '15 at 16:48
  • \$\begingroup\$ @Deduplicator Ahh, makes sense, thanks for the pointer, I'll post an answer to my own question with the new code. \$\endgroup\$ – AlexeiBarnes Dec 10 '15 at 17:09
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Jamal Dec 11 '15 at 3:02
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ OK, didn't know stl algorithms would take advantage of that but it makes sense, thanks! \$\endgroup\$ – AlexeiBarnes Dec 10 '15 at 16:49
  • \$\begingroup\$ 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. \$\endgroup\$ – AlexeiBarnes Dec 10 '15 at 17:04
  • \$\begingroup\$ @AlexeiBarnes: Yes on all counts. \$\endgroup\$ – Martin York Dec 10 '15 at 18:21
  • \$\begingroup\$ Awesome, once I have figured out some things about the copy swap idiom and SFINAE I'll post the revised code including this. \$\endgroup\$ – AlexeiBarnes Dec 10 '15 at 18:35
  • \$\begingroup\$ @AlexeiBarnes: Just do it in a new question. \$\endgroup\$ – Martin York Dec 10 '15 at 18:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.