Overload operator + for custom vector class

Question

Everyone.

I have a problem. I want to sum two vectors by overloading operator +, and assign result to third vector. But when i sum values of two vectors i save results in temporary vector, and in the end i return that vector. But when sums are done that temporary vector calls destructor and values are deallocated. How can i solve that problem?

P.S I have some problem with other operators. But i'm thinking salvation for others will be the same.

// Vector class definition
       template <class T> class Vector
       {
           private:
               unsigned x; // used to store size of vector
               T *vector;
           public:
               Vector();
               Vector(unsigned x);
               Vector(std::initializer_list<T> list);
               ~Vector();
               void random(T min, T max);
               void resize(unsigned x);
               unsigned size(void);
               T &front(void);
               T &back(void);
               void operator*=(Vector<T> &vector);
               void operator/=(Vector<T> &vector);
               void operator%=(Vector<T> &vector);
               void operator+=(Vector<T> &vector);
               void operator-=(Vector<T> &vector);
               Vector<T> operator*(Vector<T> &vector);
               Vector<T> operator/(Vector<T> &vector);
               Vector<T> operator%(Vector<T> &vector);
               Vector<T> operator+(Vector<T> &vector);
               Vector<T> operator-(Vector<T> &vector);
               T &operator[](int i);
       };

/* Vector Constructors */
template <class T> Vector<T>::Vector()
{

}

template <class T> Vector<T>::Vector(unsigned x)
{
    Vector::x = x;
    Vector::vector = new T[Vector::x];
}

template <class T> Vector<T>::Vector(std::initializer_list<T> list)
{
    Vector::x = list.size();
    Vector::vector = new T[Vector::x];

    auto it = list.begin();
    for(int i = 0; i < Vector::x; i++, it++)
        Vector::vector[i] = *it;
}

/* Destructor */
template <class T> Vector<T>::~Vector()
{
    delete[] Vector::vector;
}
template <class T> void Vector<T>::random(T min, T max)
{
    assert(std::is_arithmetic<T>::value);
    
    std::random_device rd;
    std::mt19937 eng(rd());

    if constexpr(std::is_floating_point<T>::value)
    {
        std::uniform_real_distribution<T> dist(min, max);

        for(int i = 0; i < Vector::size(); i++)
            Vector::vector[i] = dist(eng);
    }
    else
    {
        std::uniform_int_distribution<T> dist(min, max);

        for(int i = 0; i < Vector::size(); i++)
            Vector::vector[i] = dist(eng);
    }
}


template <class T> Vector<T> Vector<T>::operator+(Vector<T> &vector)
{
    assert(Vector::size() == vector.size());

    Vector<T> output(Vector::size());

    for(int i = 0; i < Vector::size(); i++)
        output[i] = Vector::vector[i] + vector[i];

    return output; // after this line output vector is deallocated :(
}

int main(int argc, char *argv[])
{
    Vector<float> *vector1 = new Vector<float>(3);
    Vector<float> *vector2 = new Vector<float>(3);
    Vector<float> *vector3 = new Vector<float>;

    vector1->random(-1.0f, 1.0f);
    vector2->random(-1.0f, 1.0f);
    *vector3 = *vector1 + *vector2;

    delete vector1;
    delete vector2;
    delete vector3; // here's SIGABORT results, even if vectors are not pointers (they will be deallocated before return anyway)

    return 0;
}

You must follow [The Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three): Define copy constructor and assignment operator to copy contents of the array instead of the pointer. — MikeCAT, May 16 '21 at 15:18
This is a [Rule of Three/Five](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) problem. You've implemented a custom destructor, so you need to implement a custom copy/move constructor and copy/move assignment operator as well. — Silvio Mayolo, May 16 '21 at 15:18
Perhaps you could use a [`std::valarray`](https://en.cppreference.com/w/cpp/numeric/valarray/valarray) instead? — Ted Lyngmo, May 16 '21 at 15:24
Maybe, you could add debug output to the constructors and the destructor. You might be surprised when and where they are called... — Scheff's Cat, May 16 '21 at 15:29
I already debugged. data got destructed when operator+ returns results. and then in main function vector3 tries to free that data again. I try rule of three as mentioned above and write results. — Darvlinig, May 16 '21 at 15:36
@Darvlinig -- Please post all of the code you claim you tried. I can tell you right now, given the code you have posted, there is no way `operator +` can work, since you are returning a `Vector` by value, and your `Vector` class has incorrect copy semantics. You should concentrate on making `Vector` copyable first, and then worry about `operator +` later. — PaulMcKenzie, May 16 '21 at 16:05

score 0 · Accepted Answer · answered May 16 '21 at 17:21

Changed Code a little bit. Thanks all for answers.

template <class T> class Vector
    {
        private:
            unsigned x; // used to store size of vector
            T *vector;
        public:
            Vector();
            Vector(unsigned x);
            Vector(std::initializer_list<T> list);
            Vector(const Vector<T> &vector);
            ~Vector();
            void random(T min, T max);
            void resize(unsigned x);
            unsigned size(void) const;
            T &front(void) const;
            T &back(void) const;
            Vector<T> operator+(const Vector<T> &vector);
            Vector<T> &operator=(const Vector<T> &vector);
            T &operator[](int i) const;
    };

/* Vector Constructors */
template <class T> Vector<T>::Vector()
{

}

template <class T> Vector<T>::Vector(unsigned x)
{
    Vector::x = x;
    Vector::vector = new T[Vector::x];
}

template <class T> Vector<T>::Vector(std::initializer_list<T> list)
{
    Vector::x = list.size();
    Vector::vector = new T[Vector::x];

    auto it = list.begin();
    for(int i = 0; i < Vector::x; i++, it++)
        Vector::vector[i] = *it;
}

/* Copy constructor */
template <class T> Vector<T>::Vector(const Vector<T> &vector)
{
    Vector::x = vector.x;
    
    Vector::vector = new T[Vector::x];
    
    for(int i = 0; i < Vector::x; i++)
        Vector::vector[i] = vector[i];
}

/* Destructor */
template <class T> Vector<T>::~Vector()
{
    delete[] Vector::vector;
}

template <class T> Vector<T> Vector<T>::operator+(const Vector<T> &vector)
{
    assert(Vector::size() == vector.size());

    Vector<T> output(vector.size());

    for(int i = 0; i < Vector::size(); i++)
        output[i] = Vector::vector[i] + vector[i];

    return output;
}

template <class T> Vector<T> &Vector<T>::operator=(const Vector<T> &vector)
{
    if(this == &vector)
        return *this;

    Vector::x = vector.x;
    
    for(int i = 0; i < Vector::size(); i++)
        Vector::vector[i] = vector[i];

    return *this;
}

template <class T> T & Vector<T>::operator[](int i) const
{
    return Vector::vector[i];
}

int main(int argc, char *argv[])
{
    Vector<int> vector1 = { 1, 2, 3 }, vector2 = vector1, vector3 = vector1 + vector2;
    std::cout << vector1 << std::endl << vector2 << std::endl << vector3 << std::endl;

    return 0;
}

Results:

darvlinig@gentoo ~/tests/test.cpp $ ./test 
[1, 2, 3] 
[1, 2, 3]
[2, 4, 6]
darvlinig@gentoo ~/tests/test.cpp $

The assignment operator is wrong. You did not delete the old memory, and allocate for the new memory. What if the passed-in `Vector` has more elements than `this`? A correct assignment operator would simply do this: `Vector temp = vector; std::swap(temp.x, x); std::swap(temp.vector, vector); return *this;` — PaulMcKenzie, May 16 '21 at 19:28
Second, there is no need prepend `Vector::` in all of those lines of code. For example `delete[] vector;` instead of `delete[] Vector::vector;` — PaulMcKenzie, May 16 '21 at 19:32
In addition to this, here is a simple one line program that fails: `int main() {Vector v;}`. The reason why it fails is that you did not initialize the member variables in the default constructor for `Vector`, thus the call to `delete[]` in the destructor is using an uninitialized pointer value. — PaulMcKenzie, May 16 '21 at 19:35
Valgrind tells that no leaks are possible and all memory was freed. Anyway i made a mistake by not checking sizes of vector when assign new values from ohter vector. thanks for your reply, I already fixed that. — Darvlinig, May 17 '21 at 20:23

Overload operator + for custom vector class

1 Answers1