Vector Implementation C++ using RAII

Summary

Broken.

• It leaks memory.
• It does not construct/destruct the object correctly.

Please have a read of the articles I wrote on building your own vector.

Have a read of the vector series: lokiastari.com/series

Code Review

There is one of these in the standard!

struct out_of_range {};

If you are going to define your own exceptions then please at least inherit from std::exception but preferably std::runtime_error. These base classes allow you to store an error message that you can use for logging.

Sure it is fine to use a standard allocator. But remember that allocators simply allocate the space they do not call the constructor or the destructor of the object that they store.

template<typename T, typename A> struct vector_base {
    A alloc;    //allocator

All relatively standard.

    T* elem;    //start of allocation
    int sz;     //number of elements
    int space;  //amount of allocated space

You only have one constructor here?

    vector_base(int n)
        :elem{alloc.allocate(n)}, sz{n}, space{n} {}

What happens when the vector is move constructed? Having this as a base class will force you to allocate space when the move constructor is not supposed to. You are supposed to move the pre-allocated storage from the source to the new vector.

Another question above this constructor.

    vector_base(int n)
        :elem{alloc.allocate(n)}, sz{n}, space{n} {}

Why is size sz set to n? You always allocate all the objects in the array on construction. That sort of defeats the purpose of pre-allocating with extra space. There is a difference between the space you have and the number of elements you have. There should be a constructor that allows the child to establish this.

Side note here:
The std::vector behaves like a normal C array. It destroys the objects contained in the reverse order. This mimics the normal behavior of RAII objects. The most recently created is the first to be destroyed.

I would use this behavior as well.

    ~vector_base() 
    {
        //for(int i = 0; i < sz; i++) alloc.destroy(&elem[i]);
        alloc.deallocate(elem, space);
        std::cout << "Vector destroyed.\n";
    } };

I don't see a default constructor ?

    vector (int size, T def = T())
        : vector_base<T,A>(size)
    {
        this->construct(def); // initialize each element to 0
        std::cout << "Vector constructed\n";
    }

What happens if I want to create an empty vector? I need to explicitly set the size to zero? Sure it's a valid design (but not what I would expect).

Delete commented out code.

    //~vector() {delete [] this->elem;}

    //{} initialization

Normally the move operators are marked noexcept.

    vector ( vector&& arg); //move constructor
    vector& operator= (vector&& arg); //move assignment

This is because if they are added to standard containers objects with noecept move operations can have certain optimizations done to them without violating the string exception gurantee.

When I grow the internal array it because I want to put more stuff in it.

    void resize(int newspace, T def = T()); //growth

The stuff I put in it may not necessarily be a default initialized T object, nor will I usually want to put the same object into all locations. Forcing me to initialize all the objects now is going to be very inefficient if I am just going to overwrite them.

You should allocate the space (increase space) but not increase size sz and not initalize the objects. When you add more items to the containre then you can initialize the objects either via copy or move.

You only support copy insertion into the contaienr.

    void push_back(const T& d);

I would add move and build in place.

    void push_back(T&& d);
    template<typename... Args>
    void emplace_back(Args&&...);

Copy only works if the elements are already constructed.

//copy constructor
template<typename T,typename A>  vector<T,A>::vector (const vector<T,A>& arg)
    :vector_base<T,A>{arg.size()} {
    std::copy (arg.elem, arg.elem + arg.sz, this->elem);
    std::cout << "Vector constructed\n";
}

Your base class allocated the space but does not initialize the objects in that space. So you have uninitialized memory there. You then try and copy into that un-initialized memory. This will invoke the copy assignment operator on the objects (this makes the assumption that the object has been constructed (which it has not).

You need to construct each if the elements. The easy way to do this is to call push_back.

//copy constructor
template<typename T,typename A>
vector<T,A>::vector(vector<T,A> const& arg)
    : vector_base<T,A>{arg.size()}
{
    // First set size to zero as your base class sets it to n.
    sz = 0;
    for (auto const& element: arg) {
       push_back(element);
    }
}

Note: push back needs to call the constructor (I will check that when I get there).

You make things hard for your self by doing this manually. Look up the copy and swap idiom.

template<typename T,typename A> 
vector<T,A>& vector<T,A>::operator= (const vector<T,A>& arg) {

    // Sure.
    // But this pattern is self deeating as normally you
    // don't do self assignment so you on average this makes the
    // copy slower because now every time you have to do this check.
    //
    // The copy and swap idium goes the other way.
    // Normally operations (which is 99.9999999999999% of them) don't pay
    // the cost for this test.
    // unfortunately on self assignment you do lots of extra work and
    // make an unnecessary copy. But on average it is still cheaper as
    // self assignment happens rareally in normal code (if it happens a lot
    // in your code then make an exception and do the test but prove that
    // you are doing a lot of self assignment first to make it worth the
    // hassle.
    if (this == &arg) return *this; //self-assignment, do nothing



    if (arg.sz <= this->space) // enough space, no need for extra allocation
    {
        // You can use copy for the first `this->sz` elements.
        // But from this->sz to arg.sz you need to initialize the elements first.
        // and if arg.sz is smaller than this->sz you will need to call
        // the destructor on the elements.
        for (int i = 0; i < arg.sz; i++) this->elem[i] = arg.elem[i]; //copy elements
        this->sz = arg.sz;
        return *this;
    }


    // Get some space.
    // Remember the allocator has not initialized the object in this
    // storage so you can not simply copy. You must initialize
    // each member (call the constructor).
    // note: calling the constructor can throw. If this happens you
    //       should make sure to release the allocated array correctly.
    T* n = this->alloc.allocate(arg.sz);
    for (int i = 0; i < arg.sz; i++) n[i] = arg.elem[i]; 

    // Before you call deallocate you must call the destructor on all the
    // elements that have been stored in this space. The allocator does not
    // do this for you.
    // Note: Calling the destructor can potentially throw. You must make sure
    //       to call deallocate even if you throw otherwise you will leak.
    this->alloc.deallocate(this->elem, this->space);

    // Make sure this still happens if there is an exception thrown.
    // otherwise your object will be in some funcky state.
    this->space = arg.space;
    this->sz = arg.sz;
    this->elem = n; //set the elem (of the current vector) to the argumen's elem
    return *this; //return a self-reference }

This move does extra allocation (why).
But then leaks it.

template<typename T,typename A>
vector<T,A>::vector (vector<T,A> && arg)
    // The base class allocates space you are not going to use.
    :vector_base<T,A>{arg.size()} {
    // You just leaked the memory allocated in the base class constructor.
    this->elem = arg.elem;

    // Sure.
    arg.elem = nullptr;
    arg.space = 0;
    arg.sz = 0;
    std::cout << "Vector constructed\n"; }

Again you are leaking memory.

template<typename T,typename A>
vector<T,A>& vector<T,A>::operator= (vector<T,A> && arg) {
    
    // You just leaked this->elem
    this->elem = arg.elem;
    this->sz = arg.sz;

    // Now you are calling the destructor
    // on the elements thus making them invalid.
    // There seems be an ordering issue here.
    // destory and release this object first.
    // then copy over the elements.
    for (int i = 0; i < arg.sz; i++) this->alloc.destroy(&this->elem[i]);
    arg.elem = nullptr;
    arg.sz = 0;
    return *this; }

Reserve should not initialize the memory.

template<typename T,typename A>  void vector<T,A>::reserve(int newspace) {
    if(newspace <= this->space) return; //never allocate less memory
    T* p = this->alloc.allocate(newspace);

    // What happens if the constructor throws an exception.?
    for(int i = 0; i < this->sz; i++) this->alloc.construct(&p[i], this->elem[i]); 
    

    // You have not called the destructor on the current elements.
    this->alloc.deallocate(this->elem, this->space);
    this->elem = p; //point elem to the newly allocated array p
    this->space = newspace; }