Overloaded Operator '=' using arrays gives a runtime error C++

Question

I'm trying to set up the overloaded operator '=' for a custom Array class for practice, but it seems to be causing a runtime error.

class Array {
    private:
        static int numberOfElements; //class static variable
        int size;
        int* numbers;

    public:
        Array(int);
        Array(const Array&);
        ~Array();

        int getSize();
        static int getNumberOfElements();

        Array& operator =(const Array&);
};

This overloaded operator function produces the correct output, but with a runtime error:

Array& Array::operator =(const Array& newArray) {
    numberOfElements = numberOfElements - size + newArray.size;
    size = newArray.size;
    for (int i = 0; i < size; i++)
        numbers[i] = newArray.numbers[i];
    return *this;
}

Before, I had

Array& Array::operator =(const Array& newArray) {
    delete[] numbers;
    numberOfElements = numberOfElements - size + newArray.size;
    size = newArray.size;
    numbers = new int[size];
    for (int i = 0; i < size; i++)
        numbers[i] = newArray.numbers[i];
    return *this;
}

Which does not produce a runtime, but creates an array filled with garbage. numberOfElements is just tracking the total elements in all Arrays, and shouldn't be a factor in the error. I'm certain the issue is the dynamic allocation, but I can't seem to logically figure out why it would throw a runtime if I'm only overwriting the original array with newArray, and why the latter is filling with garbage even though the allocated array is being set to newArray's elements.

Possibly unrelated, but make sure you call size with `.size()` instead of `.size`. — KyIe LiebIer, Feb 26 '20 at 00:18
The first version (without new/delete) obviously is incorrect as the new size might be different to the old size so you write out of bounds sometimes — M.M, Feb 26 '20 at 00:24
The second version is better (although it should check self-assignment and isn't exception-safe); if it doesn't seem to work then you probably have a bug somewhere else in the program (which might be a self-assignment call). I would recommend using the second version and if you still have trouble then post a https://stackoverflow.com/help/minimal-reproducible-example — M.M, Feb 26 '20 at 00:25
If your copy constructor works correctly, and your destructor works correctly, then the assignment operator becomes 3 lines of code using `copy / swap`. Please show your copy constructor. — PaulMcKenzie, Feb 26 '20 at 00:37
Why is `numberOfElements` static? That's a whole set of bugs all on it's own — Mooing Duck, Feb 26 '20 at 00:52
Array::Array(const Array& other) { size = other.size; numbers = new int[size]; for (int i = 0; i < size; i++) numbers[i] = other.numbers[i]; numberOfElements += size; } — Austin Chow, Feb 26 '20 at 00:56

Remy Lebeau · Answer 1 · 2020-02-26T00:48:50.857

If newArray.size is larger than this->size, your operator= trashes memory. It needs to reallocate its numbers[] array to account for the larger size, which you were originally doing (just not in an exception-safe manner).

If newArray.size is not larger than this->size, no reallocation is needed, just copy the values from newArray.numbers into this->numbers as-is, making sure to not exceed the smaller of newArray.size or this->size.

Try something more like this:

Array& Array::operator=(const Array& newArray)
{
    if (&newArray != this)
    {
        if (size != newArray.size)
        {
            numberOfElements -= size;

            delete[] numbers;
            numbers = NULL;
            size = 0;

            numbers = new int[newArray.size];
            size = newArray.size;

            numberOfElements += size;
        }

        for (int i = 0; i < size; ++i)
            numbers[i] = newArray.numbers[i];
    }

    return *this;
}

Which can then be made safer using the copy-swap idiom using your existing copy constructor:

Array::Array(int num)
{
    size = num;
    numbers = new int[num];
    numberOfElements += num;
}

Array::Array(const Array &srcArray)
{
    size = src.size;
    numbers = new int[size];
    for (int i = 0; i < size; ++i)
        numbers[i] = srcArray.numbers[i];
    numberOfElements += size;
}

Array::~Array()
{
    delete[] numbers;
    numberOfElements -= size;
}

#include <algorithm>
Array& Array::operator=(const Array& newArray)
{
    if (this != &newArray)
    {
        if (size != newArray.size)
        {
            Array tmp(newArray);
            std::swap(numbers, tmp.numbers);
            std::swap(size, tmp.size);
        }
        else
        {
            for (int i = 0; i < size; ++i)
                numbers[i] = newArray.numbers[i];
        }
    }

    return *this;
}

Overloaded Operator '=' using arrays gives a runtime error C++

1 Answers1