Stack overflow error when having an assignment operator and a copy constructor

Question

I have an error with an assignment operator and copy constructor. It shows a stack overflow error in Visual Studio 2022.

#include<iostream>

using namespace std;

class A
{
    int* x;
    A copy(const A& other)
    {
        x = new int[10];
        for (int i = 0; i < 10; i++)
            x[i] = other.x[i];
        return *this;
    }

public:
    A()
    {
        x = new int[10];
    }
    A(A& other)
    {
        copy(other);
    }
    const A& operator=(const A& other)
    {
        if (this != &other)
        {
            delete[]x;
            copy(other);
        }
        return *this;
    }
};

int main()
{
    A o1, o2;
    o1 = o2;
    return 0;
}

Think about what happens when your `copy` function returns `*this` *by value* (and create a *copy*)... — Some programmer dude, May 24 '23 at 12:10
Also note that your copy constructor should take a `const` reference — Alan Birtles, May 24 '23 at 12:16
To be able to accept const and/or temporary objects. Why did you write `const` in the copy assignment but not in the copy constructor? — HolyBlackCat, May 24 '23 at 12:20
I notice a very distinct absence of a destructor in this class, so memory leakage is guaranteed. Also, if you're going to hard-code an array of 10 integers, why not make `x` an array instead of a pointer? — paddy, May 24 '23 at 12:20
`=` should be `A & operator=(const A& other)`, ie no `const` on the return. On the other hand, your `copy` doesnt need chaning, it can return `void` — 463035818_is_not_an_ai, May 24 '23 at 12:21
Why not replace `x = new int[10];` with `std::vector` it will avoid a lot of your problems — Pepijn Kramer, May 24 '23 at 12:41

score 0 · Answer 1 · answered May 24 '23 at 12:54

The comments answered your particular question, but there are a few other issues you may want to consider.

If you define a copy constructor and copy assignment operator, you probably also need to define a destrcutor. This is known as the rule of three/five. In the posted code that means you need a destructor to delete the allocated memory.

Also, in order to ensure exception safety, you may also want to consider using the copy and swap idiom. This also allows the copy assignment operator to be written in terms of the copy constructor eliminating the need to write a separate copy function that is used by both.

Lastly, in modern C++ there are very few instances that call for using raw pointers. Since this class presumably owns the data in x, a std::unique_ptr would accurately indicate the intended owership and simplify managing the memory.

Sample Code

// Rule of three with copy-and-swap idiom
class A {
public:
    A() {
        x = new int[10];
    }

    A(const A& other) : A() {
        std::copy(other.x, other.x + 10, x);
    }

    const A& operator=(A other) {
        std::swap(*this, other);
        return *this;
    }

    ~A() {
        delete x;
    }

private:
    int *x{nullptr};
};

// Copy-and-swap idiom using a unique pointer
class AUP {
public:
    AUP() {
        x = std::make_unique<int[]>(10);
    }

    AUP(const AUP& other) : AUP() {
        std::copy(other.x.get(), other.x.get() + 10, x.get());
    }

    const AUP& operator=(AUP other) {
        std::swap(*this, other);
        return *this;
    }

private:
    std::unique_ptr<int[]> x;
};

int main() {
    A a;
    A b = a;
    AUP aup;
    AUP bup = aup;
}

Stack overflow error when having an assignment operator and a copy constructor

1 Answers1

Sample Code