C ++ copy constructor with pointers
I understand the need for deep copy pointers (in cases where you want a complete copy of an object), my confusion comes with the following (fully compiled example).
#include "stdafx.h"
#include <string>
class a
{
public:
a::a(std::string _sz) :
m_sz(_sz)
,m_piRandom(new int)
{
*m_piRandom = 1;
};
~a()
{
delete m_piRandom;
m_piRandom = NULL;
};
a::a(const a &toCopy)
{
operator=(toCopy);
}
a& a::operator=(const a &toAssign)
{
if (this != &toAssign)
{
m_sz = toAssign.m_sz;
if (m_piRandom)
{
// Need to free this memory!
delete m_piRandom;
m_piRandom = NULL;
}
m_piRandom = new int(*toAssign.m_piRandom);
}
return *this;
}
void SetInt(int i)
{
if (!m_piRandom)
{
m_piRandom = new int;
}
*m_piRandom = i;
}
private:
std::string m_sz;
int* m_piRandom;
};
int _tmain(int argc, _TCHAR* argv[])
{
a Orig = a("Original");
a New = a("New");
New.SetInt(9);
New = Orig;
return 0;
}
Now, in my example, I want to test a scenario where I have an object with memory allocated to it, in this case:
a New = a("New");
New.SetInt(9); // This new the memory
allocates memory and then when we say: New = Orig;
I would expect a memory leak because if I were blindly new'd m_piRandom = new int(*toAssign.m_piRandom);
I would lose the memory I mentioned earlier.
Therefore, I decided to put the following in the assignment operator:
if (m_piRandom)
{
// Need to free this memory!
delete m_piRandom;
m_piRandom = NULL;
}
This causes the code to fail when calling the next (first line!) a Orig = a("Original");
Because it calls the copy constructor (which I call for the assignment operator for less duplication) and the pointer is m_piRandom
set to 0xcccccccc. Not zero. Therefore, it tries to delete memory that has never been allocated. I expect it to work when it gets to New = Orig;
, because it will remove it first before assigning a copy. Can anyone shed some light on this, I think my biggest problem is that m_piRandom is not NULL, I also tried to define a default constructor for which NULL indicates a default pointer, but that didn't help. Apologies for the completely contrived code.
thank
source to share
Your first mistake is that you have implemented the copy constructor in terms of an assignment operator. The copy constructor creates a new object based on another object, while the assignment operator clears and changes a bit of the already created object.
So your correct copy constructor would be: -
a::a(const a &toCopy) : m_sz(toCopy.m_sz), m_piRandom(new int)
{
*m_piRandom = toCopy.m_piRandom;
}
After that, you can simplify your assignment operator:
a& a::operator=(const a &toAssign)
{
if (this != &toAssign)
{
m_sz = toAssign.m_sz;
if (m_piRandom) //<<<<< No need to check this as it should always be
{ //<<<<< initialized by constructors.
delete m_piRandom;
m_piRandom = NULL;
}
m_piRandom = new int(*toAssign.m_piRandom);
}
return *this;
}
After removing this redundancy, your assignment operator looks like
a& a::operator=(const a &toAssign)
{
if (this != &toAssign)
{
m_sz = toAssign.m_sz;
m_piRandom = new int(*toAssign.m_piRandom);
}
return *this;
}
source to share
The error occurs because the copy constructor does not initialize m_piRandom
. This means that the variable will (most likely) be filled with garbage (everything that was in the memory location when the object was initialized).
The sequence of calls is as follows:
a::a() [doesn not initialize m_piRandom] -> a::operator= -> delete m_piRandom.
To fix it:
a::a(const a &toCopy)
: m_piRandom{ nullptr } // <---- add this
{
operator=(toCopy);
}
Edit : You can greatly improve the assignment operator by using copy & swap idiom .
source to share