HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simplified implementation of std::vector in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stdsimplifiedimplementationvector

Problem

I implemented a simplified version of a C++ std::vector/ArrayList learning from this link. From it, I tried to write an ArrayList class rather than a Vector. In my ArrayList implementation, I didn't add iterators, operator[], emplace and a lot of other stuff because my main focus is to practice the following aspects:

  • My ArrayList class guarantees strong exception safety, using the copy and swap idiom



  • The container still works if T passed in is not default-constructible



  • Correctness of the implementation of the Rule of Five



  • General correctness and efficiency (i.e: No memory leaks, dangling pointers, etc.)



Because of those 4 areas I'd like to focus on practicing, I only had extra add and remove method in the class.

Please focus on those 4 areas I mentioned for reviewing my code, and any other important style choice errors or any other problems that I missed.

```
using namespace std;

template
class ArrayList
{
public:
ArrayList();
ArrayList(int size);
ArrayList(const ArrayList& other);
ArrayList(ArrayList&& other);
~ArrayList();

ArrayList& operator= (const ArrayList& other);
ArrayList& operator= (ArrayList&& other);

void add(const T& item);
void add(T&& item);
void remove(int index);

friend void swap(ArrayList& A, ArrayList& B)
{
using std::swap;
swap(A.actualSize, B.actualSize);
swap(A.allocatedSize, B.allocatedSize);
swap(A.arr, B.arr);
}

private:
T* arr;
size_t allocatedSize;
size_t actualSize;

void resize();
void addInternal(const T& item);
void addInternalMove(T&& item);
};

template
ArrayList::ArrayList()
{
arr = static_cast(::operator new(sizeof(T)*100));
actualSize = 0;
allocatedSize = 100;
}

template
ArrayList::ArrayList(int size)
{
if(size (::operator new(sizeof(T)*size));
actualSize = 0;
allo

Solution

Here's the original code with comments inserted:

Change this constructor to use size_t:

ArrayList(int size);


Should be:

ArrayList(size_t size);


Change parameter of remove to be a size_t.

void remove(int index);


Should be:

void remove(size_t index);


Eliminate redundant using

friend void swap(ArrayList& A, ArrayList& B)
        {
            using std::swap;
            // --snip--
        }


Include named constant for 100:

private:
        // --snip--
        const size_t defaultSize = 100;
};


Avoid repetitive code, implement ArrayList::ArrayList() in terms of
ArrayList::ArrayList(size_t).

template 
ArrayList::ArrayList()
    : ArrayList::ArrayList(ArrayList::defaultSize)
{
}


Avoid ::operator new(0) and allocatedSize == 0

template 
ArrayList::ArrayList(int size)
{
    if(size < 0)
        throw;

    // --snip--
}


Should be:

template 
ArrayList::ArrayList(size_t size)
{
    if(size < 1)
        throw;

    // --snip--
}


Syntax error: size is undefined. You mean to use other.actualSize when
setting actualSize, but really, you need to use 0 since add/addInternal
will increment actualSize. You can safely use addInternal since
allocatedSize is set appropriately large.

template 
ArrayList::ArrayList(const ArrayList& other)
{
    arr = static_cast(::operator new(sizeof(T)*size));
    allocatedSize = other.allocatedSize;
    actualSize = actualSize;

    for(size_t i = 0; i<other.actualSize; i++)
        add(other.arr[i]);
}


Should be:

template 
ArrayList::ArrayList(const ArrayList& other)
{
    arr = static_cast(::operator new(sizeof(T)*other.allocatedSize));
    // --snip--
    actualSize = 0;
    // --snip--
        addInternal(other.arr[i]);
}


Really, I'd delegate to ArrayList(size_t)

template 
ArrayList::ArrayList(const ArrayList& other)
    : ArrayList::ArrayList(other.allocatedSize)
{
    for(size_t i = 0; i<other.actualSize; i++)
        addInternal(other.arr[i]);
}


Syntax error: method should return a value. other will be uninitialized at
the exit to the method and destructor won't work.

template 
ArrayList::ArrayList(ArrayList&& other)
{
    swap(*this, other);
}


Could be:

template 
ArrayList::ArrayList(ArrayList&& other)
    : ArrayList::ArrayList(1)
{
    swap(*this, other);
    return *this;
}


But this is more efficient:

template 
ArrayList::ArrayList(ArrayList&& other)
{
    actualSize = other.actualSize;
    allocatedSize = other.allocatedSize;
    arr = other.arr;

    // set other.arr to a value that doesn't incur the
    // cost of a new, but can be deleted without heap
    // corruption.
    other.arr = nullptr;

    // avoid calling destuctor on elements of the array
    // in destructor call on other.
    other.actualSize = 0;

    return *this;
}


Add a line break after for loop.

template 
ArrayList::~ArrayList()
{
    for(size_t i = 0; i<actualSize; i++)
    {
        arr[i].~T();
    }::operator delete(arr);
}


Should be:

template 
ArrayList::~ArrayList()
{
    for(size_t i = 0; i<actualSize; i++)
        arr[i].~T();

    ::operator delete(arr);
}


Add return *this;:

template 
ArrayList& ArrayList::operator =(ArrayList&& other)
{
    swap(*this, other);
    return *this;
}


If you hit an exception in the for loop, this is corrupt since you mutate
allocatedSize.

template 
void ArrayList::resize()
{
    allocatedSize *= 2;
    ArrayList tmp(allocatedSize);
    // --snip--
}


Should be:

template 
void ArrayList::resize()
{
    ArrayList tmp(allocatedSize * 2);
    // --snip--
}


move is redundant; item is already a T&& so the cast done by move is a
no-op.

template 
void ArrayList::add(T&& item)
{
    // --snip--
    addInternalMove(move(item));
}


Should be:

template 
void ArrayList::add(T&& item)
{
    // --snip--
    addInternalMove(item);
}


If T does not define T::T(T&& other), addInternalMove won't work and the
compiler will not be able to implicitly fall back to the version of
addInternal with copy semantics. Consider renaming addInternalMove to
just addInternal.

This code has a bunch of flaws. actualSize is not decremented. It's not
exception safe. You call the detructor on arr[i], but then assign it. You're
moving things "up" rather than "down". It's inefficient to loop from 0 to
index doing a no-op. actualSize-1 has interger underflow if actualSize is 0.

I'd rewrite:

template 
void ArrayList::remove(int index)
{
    if(indexactualSize - 1)
        throw;

    for(size_t i=0; i i)
            arr[i+1] = arr[i];
    }
}


As:

```
template
void ArrayList::remove(size_t index)
{
// Allow array to shrink. It's possible this is not desired.
size_t newSize = allocatedSize / 2;

if (newSize tmp(newSize);

if(index + 1>actualSize)
throw;

for(size_t i=0; i<index; i

Code Snippets

ArrayList(int size);
ArrayList(size_t size);
void remove(int index);
void remove(size_t index);
friend void swap(ArrayList& A, ArrayList& B)
        {
            using std::swap;
            // --snip--
        }

Context

StackExchange Code Review Q#136143, answer score: 4

Revisions (0)

No revisions yet.