current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

In some (lower-level) parts of my codebase I often have the need to allocate resizable array storage for certain objects, that usually are not default-constructible.

I created a simple class using std::aligned_storage and I was wondering if it's safe and if it can be improved.

The user of the class has to keep track of the current capacity.

template<typename T> class ResizableArray
{
    private:
        using TStorage = std::aligned_storage_t<sizeof(T), alignof(T)>;
        TStorage* data{nullptr};

    public:
        ~ResizableArray() { delete[] data; }

        void resize(std::size_t mSizeOld, std::size_t mSizeNew)
        {
            auto newData(new TStorage[mSizeNew]);
            for(auto i(0u); i < mSizeOld; ++i) newData[i] = std::move(data[i]);

            delete[] data;
            data = newData;
        }

    auto& operator[](std::size_t mIdx) noexcept 
    { return reinterpret_cast<T&>(data[mIdx]); }

    const auto& operator[](std::size_t mIdx) const noexcept 
    { return reinterpret_cast<const T&>(data[mIdx]); }
};

Example usage:

int main()
{
    ResizableArray<Thing> a;

    // Resize array capacity from 0 to 10
    a.resize(0, 10);

    for(auto i(0u); i < 10; ++i)
        new (&a[i]) Thing{/* something */};

    return 0;
}
share|improve this question
    
Why not using std::vector::reserve() and std::vector::emplace_back()? vector<T> does require T to be default-constructible. –  firda 3 hours ago

3 Answers 3

up vote 5 down vote accepted

I see a couple of problems:

  • If you ever shrink the array, the loop for copying the elements will overrun the bounds of newdata because the index runs from 0 to mSizeOld.

  • You don't store the size of the array in the class, so code like this will lose data:

    ResizeableArray<Thing> a;
a.resize(0, 10);
// Populate the array
a.resize(0, 11);
// Now a is empty again because resize() uses the first argument.

    This might be intentional — to allow you to start over with a fresh set of data &mdash but if so, resize() should be commented to say that.

share|improve this answer
    
Thanks for the review. The array is meant to never be shrunk - I will probably rename it to GrowableArray or something similar. I added an assertion to make sure mSizeOld <= mSizeNew. Not storing the size of the array is intentional (as not calling the items' destructors is) - I will make sure to document that properly. –  Vittorio Romeo 5 hours ago
up vote 5 down vote

Ciao Vittorio,

You're looking for safety and improvements, thus I'll give a brief overview of the code and then focus on them.

Overview

  1. You're abusing auto: in a lot of places, you don't really need it such as in for loops.
  2. I can't see any library #include but you'll need at least <memory> and <cstdint>.
  3. Your code will not compile for non DefaultConstructible classes.
  4. There is no way to get the number of elements.
  5. Following #4, it's hard to extract a range from it and/or iterate over it.

Safety

The resize function has some faults:

  1. There is no check against mSizeOld and mSizeNew, therefore mSizeOld can easily be > mSizeNew. mSizeOld should be a member of the class.
  2. If mSizeNew == mSizeOld, elements will be however copied/moved.

Improvement

  1. You could make resize a variadic template in order to create elements with such arguments.
  2. Check for mSizeOld and mSizeNew and throw out_of_range/ if you find something wrong.
share|improve this answer
up vote 4 down vote

It seems to me that it would be beneficial to store the length of the array as well. It is such a tiny increase in the size of the structure that I really don't see why wouldn't you do it.

If you do keep track of the size, then you could do debug bounds checking inside operator [], which I consider very important.

As it was mentioned, there is a bug in the for loop of resize(), as the new array is allocated with mSizeNew entries and the loop will visit 0 to mSizeOld-1 entries. There is nothing preventing mSizeOld from being greater than mSizeNew.

You could also use a unique_ptr to manage the memory so that you don't have to bother writing an explicit destructor.

Apart from that, good code.

share|improve this answer
1  
(Copy-paste from the above answer) Thanks for the review. The array is meant to never be shrunk - I will probably rename it to GrowableArray or something similar. I added an assertion to make sure mSizeOld <= mSizeNew. Not storing the size of the array is intentional (as not calling the items' destructors is) - I will make sure to document that properly. –  Vittorio Romeo 5 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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