fixing shallow copy with a copy constructor

Question

I defined a copy constructor to avoid having a shallow copy of the data member n, but it is not working, v2 is still getting changed when I change v1, what I am doing wrong?

#include<iostream>
using namespace std;
class Vector
{
  public:
  int *n;

  Vector();
  Vector(const Vector& vector );

};
Vector::Vector()
{
  cout<<"Constructor called"<<endl;
}
Vector::Vector(const Vector& vector )
{
  n=new int;
  n=vector.n;
  cout<<"Copy constructor called"<<endl;
}



int main()
{
  Vector v1;
  int x=5;
  v1.n=&x;
  Vector v2=v1;

  cout <<"Vector v1 has n value: "<<*v1.n<<endl;
  cout <<"Vector v2 has n value: "<<*v2.n<<endl;
  *v1.n=499; 
  cout <<"Vector v1 has n value: "<<*v1.n<<endl;
  cout <<"Vector v2 has n value: "<<*v2.n<<endl;

  return 0;
}

You have a memory leak. The address stored in `v1.n` is being saved to `v2.n`. The memory allocated with `new int` is lost. So they are now pointing to the same memory. If you just want to copy the integer value, you need to do `*n = *vector.n`. But then you're not doing a shallow copy. You're doing a deep copy. — JohnFilleau, Jun 08 '20 at 23:13
You'll need to implement it properly. You allocate n, then leak the memory by copying the pointer, doing what the default copy does. You should copy the content of vector.n in what you allocated. For example, like this: `*n = *vector.n`. You should also implement the = operator if you implement the copy contructor. — Adrian Roman, Jun 08 '20 at 23:13
^^^ And the destructor. Add move assignment and move constructor if you're feeling squirrely. — JohnFilleau, Jun 08 '20 at 23:14
should I allocate memory anyway? I was not sure if I had to put 'n=new int;' — some_math_guy, Jun 08 '20 at 23:16
Also read up on "rule of three" or (C++11 and later) rule of five. In short, the rule of three says that, if you class needs one of a copy constructor, assignment operator, or destructor to be manually defined, all of them need to be defined. Rule of five extends that to include move construction and move assignment. — Peter, Jun 08 '20 at 23:47
You know I'm actually still kind of confused here. Do you want a shallow copy or a deep copy? What's the final goal? — JohnFilleau, Jun 09 '20 at 01:59
@JohnFilleau I wanted a deep copy, the goal was to understand why is necessary to have a copy constructor to avoid the shallow copy problem — some_math_guy, Jun 12 '20 at 22:25

Remy Lebeau · Accepted Answer · 2020-06-08T23:48:04.227

The problem is that your copy constructor is implemented wrong. You try to fix the shallow copy issue, but your code includes a statement that re-introduces the same issue again.

On this line:

n=new int;

You correctly allocate a new int, which is fine.

But on this line:

n=vector.n;

You are throwing away the int you just allocated, and instead point n to the same int that the other vector's n points to. This is the shallow copy issue all over again.

To fix the issue properly, you need to copy the value of the other vector's n member into your newly allocated n member, eg:

n = new int;
*n = *(vector.n);

Or:

n = new int( *(vector.n) );

That being said, to fully comply with the "Rule of 3", you also need to add a destructor to free n (which means you can't make v1 point to a local int variable in main()), as well as add a copy assignment operator to copy the value of n (similar to the copy constructor). In C++11, you can optionally also comply with the "Rule of 5" by adding a move constructor and move assignment operator, as well.

Try this instead:

#include <iostream>
#include <utility>
//using namespace std; // <-- bad practice!

class Vector
{
private:
  int *m_n;

public:
  Vector(int x = 0);
  Vector(const Vector &src);
  Vector(Vector &&src);
  ~Vector();

  Vector& operator=(const Vector &src);
  Vector& operator=(Vector &&src);
  // alternatively, both operators can be merged into one:
  // Vector& operator=(Vector src);

  // you should not give outside code directly access to the int* itself,
  // that would promote bad practices, and potentially crash your class's
  // internal code if outside code assigns a bad pointer.
  //
  // these accessors provide safer use of the *value* that the int* points
  // at, let the class itself be the sole handler of the int* itself ...
  int n() const;
  void n(int x);
};

Vector::Vector(int x)
  : m_n( new int(x) )
{
  std::cout << "Converting Constructor called" << std::endl;
}

Vector::Vector(const Vector &src)
  : m_n( new int(src.n()) )
{
  std::cout << "Copy constructor called" << std::endl;
}

Vector::Vector(Vector &&src)
  : m_n( src.m_n )
{
  src.m_n = nullptr;
  std::cout << "Move constructor called" << std::endl;
}

Vector::~Vector()
{
  delete m_n;
  std::cout << "Destructor called" << std::endl;
}

Vector& Vector::operator=(const Vector &src)
{
  if (&src != this) {
    *m_n = src.n();
  }
  std::cout << "Copy assignment called" << std::endl;
  return *this;
}

Vector& Vector::operator=(Vector &&src)
{
  Vector tmp(std::move(src));
  std::swap(m_n, tmp.m_n);
  std::cout << "Move assignment called" << std::endl;
  return *this;
}

/* alternatively:
Vector& Vector::operator=(Vector src)
{
  std::swap(m_n, src.m_n);
  std::cout << "Assignment called" << std::endl;
  return *this;
}
*/

int Vector::n() const
{
  return *m_n;
}

void Vector::n(int x)
{
  *m_n = x;
}


int main()
{
  Vector v1(5);
  Vector v2 = v1;

  std::cout << "Vector v1 has n value: "<< v1.n() << std::endl;
  std::cout << "Vector v2 has n value: "<< v2.n() << std::endl;

  v1.n(499);

  std::cout << "Vector v1 has n value: " << v1.n() << std::endl;
  std::cout << "Vector v2 has n value: "<< v2.n() << std::endl;

  return 0;
}

The same thing that the copy constructor is doing, but on an already existing `Vector` object. For example: `Vector v2 = v1;` is copy construction, but `Vector v2; v2 = v1;` is copy assigment. In fact, it is very common to implement a copy assignment operator using the copy constructor internally (see the [copy-swap idiom](https://stackoverflow.com/questions/3279543/)) — Remy Lebeau, Jun 08 '20 at 23:50

fixing shallow copy with a copy constructor

1 Answers1