C++ copy constructor with pointers

Question

I understand the need to deep copy pointers (in cases when you want a complete copy of an object), my confusion comes with the following (completely made up 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 the scenario where I have an object with some memory allocated to it, in this case:

a New = a("New"); 
New.SetInt(9); // This new's the memory

allocates the memory and then when we say: New = Orig; I would expect a memory leak because if I blindly new'd the m_piRandom = new int(*toAssign.m_piRandom); I would have lost the memory it was previously pointing to..

So 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 crashes the code when the following is called (first line!) a Orig = a("Original"); as it calls the copy constructor (which I call the assignment operator for less duplication) and the pointer m_piRandom is set to 0xcccccccc. Not NULL. Therefore it tries to delete memory that was never allocated. I would expect it to work when it got to the New = Orig; because it would delete it first before assigning the copy. Can anybody shed any light on this, I guess my biggest concern is that m_piRandom is not NULL, I also tried defining a default constructor for a which NULLs the pointer by default but this didn't help. Apologies for the completely contrived code..

Thanks

Answer 1

Your first mistake is that you implemented the copy constructor in terms of the assignment operator. The copy constructor brings a new object into existence based on some other object, whereas the assignment operator clears and changes the bits of an already created object.

So, your proper copy constructor would be:-

a::a(const a &toCopy) : m_sz(toCopy.m_sz), m_piRandom(new int)
{
    *m_piRandom = toCopy.m_piRandom;
}

After implementing this 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 these redundancies, 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;
}