1

I was recently implementing the copy constructor and copy assignment operator for a class that manages a dynamically allocated object, and learnt about the copy-and-swap idiom (thanks to this great answer). I ended up with the following implementation, which I believe is correct and satisfies me well :

class MyArray {
    int* arr;
    int size;

  public:
     ...

    ~MyArray() { delete[] arr; }

    // copy constructor
    MyArray(const MyArray& other) {
        size = other.size;
        arr = new int[size];
        for (int i = 0; i < size; ++i) {
            arr[i] = other.arr[i];
        }
    }

    friend void swap(MyArray& a1, MyArray& a2);

    // copy assignment operator, using copy-and-swap idiom
    MyArray& operator=(const MyArray& other) {
        MyArray tmp{other};
        swap(tmp, *this);
        return *this;
    }
};

void swap(MyArray& a1, MyArray& a2) {
    std::swap(a1.size, a2.size);
    std::swap(a1.arr, a2.arr);
}

This works well and is pretty clean (I know using smart pointers would be even better, but it is not the topic of this question).

Still, I am wondering why it is rarely mentioned that you can just implement the copy assignment operator, and then call it inside the copy constructor, like so :

class MyArray {
    int* arr;
    int size;

  public:
    ...

    ~MyArray() { delete[] arr; }

    // copy assignment operator
    MyArray& operator=(const MyArray& other) {
        if (this != &other) {
            delete[] arr;
            size = other.size;
            arr = new int[size];
            for (int i = 0; i < size; ++i) {
                arr[i] = other.arr[i];
            }
        }

        return *this;
    }

    // copy constructor just calling the copy assignment operator
    MyArray(const MyArray& other) : arr(nullptr) { *this = other; }
};

The main pitfall of this approach I can think of is if you forget to assign arr to nullptr in the copy constructor before the call to the copy assignment operator, you end up with a segmentation fault when it tries to delete[] arr.

Still, are there other reasons this would be considered bad practice ? The copy-and-swap idiom is definitely more verbose, so why is it always preferred ?

Ewaren
  • 1,343
  • 1
  • 10
  • 18
  • 1
    If you prefer coding this there's nothing wrong with it. Downsides are: (1) you don't have `swap` which is often useful (depending on the use-case), (2) you need to have a valid "empty" object the copy constructor can create before calling `operator =` on it. (3) in your code you have implemented the desctuctor twice once in `~MyArray` and once in `operator =` – al3c Oct 15 '20 at 16:02
  • @al3c point 2) is the pitfall I mentioned I think (I spent 3 hours debugging yesterday because of it, until I finally understood what was going on) ; point 3) is pretty interesting, for a small object like the one in my example it does not result in anything very bad, but I could see the code redundancy being troublesome with bigger objects. – Ewaren Oct 15 '20 at 16:09

0 Answers0