Correctly overload assignment operator for classes with pointer members

Question

Correct me if I'm wrong:

I understand that when having a class with members that are pointers, a copy of a class object will result in that the pointers representing the same memory address. This can result in changes done to one class object to affect all copies of this object.

A solution to this can be to overload the = operator. Given the example below, with an attempt to create a dynamic array class, why does making changes to MyArray1 change MyArray2:

Array Class:

#include <iostream>
#include <cstdlib>

class Array
{
public:
    Array(int N){               //constructor sets size of array
         size = N;
         arr = new int[N];
    }

    ~Array();

    int size;    //array elements
    int *arr;    //dynamic array pointer

    //fill array with random values between 1 and 100
    void fillArray() {
         for (size_t i = 0; i < size; i++)
              {arr[i] = std::rand()%100;}
    }

    //print out array to console
    void printArray() {
         for (size_t i = 0; i < size; i++)
             { std::cout << arr[i] << " ";}
             std::cout << std::endl;
    }

    //overload = operator
    Array &operator=(Array arr2) {
        std::swap(size, arr2.size);
        std::swap(arr, arr2.arr);
        return *this;
    }
};

Main.cpp:

 #include "Array.h"
 #include <iostream>

int main(){
    Array MyArray1(8), MyArray2(8);

    MyArray1.fillArray();
    MyArray2 = MyArray1;

    std::cout << "Print out arrays:" << std::endl;
    std::cout << "MyArray1: "; MyArray1.printArray();
    std::cout << "MyArray2: "; MyArray2.printArray();
    std::cout << std::endl;

    MyArray1.arr[5] = 1000;
    std::cout << "MyArray2: "; MyArray2.printArray();

    MyArray1.fillArray();
    std::cout << "MyArray2: "; MyArray2.printArray();    

    return 0;
}

Example output:

Print out arrays:
MyArray1: 41 67 34 0 69 24 78 58
MyArray2: 41 67 34 0 69 24 78 58

MyArray2: 41 67 34 0 69 1000 78 58
MyArray2: 62 64 5 45 81 27 61 91

As seen above, changes made to MyArray1, changes MyArray2. I assume the overloading of = is wrong, but how would I write it correctly?

SOLUTION:

Thanks to Chris Dodd in the comments, I realized it's just to implement a copy constructor like this in my class:

Array(const Array &arr2){
        size = arr2.size;
        arr = new int[size];

        for (size_t i = 0; i < size; i++)
        {
            arr[i] = arr2.arr[i];
        }
}

Yes, I was realising that, that's why I deleted my comment same time as you anwered! Thank you, I will give it a shot:) — remi
– remi, Commented Sep 8, 2016 at 6:43

slawekwin · Accepted Answer · 2016-09-08 09:40:43Z

10

There is more than one issue with your code. Most importantly, as pointed out in the comment to to the question, except for writing your own assignment operator, you need to write your copy constructor as well (and implement a destructor).

Second thing is, that your assignment operator takes argument by value instead of by reference, which causes default copy constructor to create a copy of your MyArray1 before it's passed to assignment operator. This is where the direct source of your problem lays.

Another thing is that your assignment operator behaves like a move assignment instead of copy assignment, meaning it replaces the original item's values with its current (default) value.

Lastly, you really want to implement the destructor, so that it deletes your array instead of just leaking it.

Summing up, you want something like this (edit: copy assignment checks inspired by Timothy's answer):

class Array
{
public:
    Array(int N)
    {
         size = N;
         arr = new int[N];
    }

    //destructor
    ~Array()
    {
        delete[] arr;
    }

    //copy constructor
    Array(const Array& arr2)
    {
        size = arr2.size;
        arr = new int[size];
        std::memcpy(arr, arr2.arr, size);
    }

    //overload = operator
    Array& operator=(const Array& arr2) 
    {
        if (this == &arr2)
            return *this; //self assignment
        if (arr != NULL)
            delete[] arr; //clean up already allocated memory

        size = arr2.size;
        arr = new int[size];
        std::memcpy(arr, arr2.arr, size);
        return *this;
    }

private:
    int size;    //array elements
    int *arr;    //dynamic array pointer
};

edited Sep 8, 2016 at 9:40

answered Sep 8, 2016 at 6:49

slawekwin

6,3101 gold badge46 silver badges58 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

remi Over a year ago

I noticed by changing my assignement operator to take a reference, and reruning the code, MyArray1 "loses" all it's information after doing: MyArray2 = MyArray1

juanchopanza Over a year ago

"Second thing is, that your assignment operator takes argument by value instead of by reference" It is the copy any swap idiom.

slawekwin Over a year ago

@juanchopanza yes, you are obviously right, I doubt OP understood how it worked, though (because of not implementing copy constructor)

user786 Over a year ago

can u please explain why compiler says

error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]    31 |                       if (this == &arr2)

Timothy Murphy · Accepted Answer · 2016-09-08 08:02:02Z

1

So, I see three issues.

You should really have a copy constructor. You never know what optimizations your compiler might make (changing assignment to copy-constructor, etc...).
you should check for self-assignment when assigning. When doing the assignment you want to get rid of your old data and replace it with the new. If you do self assignment, you'll run into errors (aka delete your data before you reassign to yourself) plus it's extra work you don't need to do.
you don't handle your dynamically allocated data when you call the destructor. This will cause a memory-leak and for long running programs becomes a large problem.

Code Updates:

~Array(){
  if (arr != NULL)
    delete arr;
}

Array(const Array& arr2){
  size = arr2.size;
  arr = new int[Size];
  for (int i = 0; i < n; ++i){ //Copy each array element into new array
    arr[i] = arr2.arr[i];
  }
}

Array& operator= (const Array& arr2){
  if (this == &arr2){ return *this; } //make sure you aren't self-assigning
  if (arr != NULL){ delete arr; } // get rid of the old data
  //same as copy constructor from here
  size = arr2.size;
  arr = new int[size];
  for (int i = 0; i < n; ++i){
    arr[i] = arr2.arr[i];
  }
  return *this;
}

edited Sep 8, 2016 at 8:02

answered Sep 8, 2016 at 6:57

Timothy Murphy

1,3628 silver badges17 bronze badges

3 Comments

juanchopanza Over a year ago

Re. point 2, OP is using the copy and swap idiom. That would be robust against self-assignment. But it is broken because of the copy constructor.

Timothy Murphy Over a year ago

Fair enough. Although for large arrays that causes a lot of extra work for self assignment.

DigviJay Patil Over a year ago

You should use delete [] for array deletion.

Saman Salehi · Accepted Answer · 2021-12-25 07:02:22Z

0

you can avoid all of these if you want to make = only but not Array MyArray1=MyArray2 because it also needs a copy constructor you can do this

Array& operator =(Array&arr)

passing by reference solve the problem but if you want to do

Array MyArray1(MyArray2) ;  or     Array MyArray1=MyArray2

you need a copy constructor

and if you want selfassignment you must check

if(this=&arr) {return *this;}

edited Dec 25, 2021 at 7:02

Saman Salehi

1,0341 gold badge14 silver badges19 bronze badges

answered Dec 24, 2021 at 0:38

Mohamed Hamdy

1

Collectives™ on Stack Overflow

Correctly overload assignment operator for classes with pointer members

3 Answers 3

4 Comments

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related