forGreatJustice

I’ve recently been working on a project that involved a whole load of code that wasn’t written by myself or anyone at the company I work for. Suffice to say, there’s a fair few problems. Anyway, I’ve been stuck on a bug that’s been stringing me a long for the past week now. At first I thought it was a multithreading problem as we’ve had to convert all the original POSIX threading code to Windows Threads. Turns out however that it was simply a case of bad memory management, which brings me to the subject of this post: The Gang Of Three.

The actual cause of the rogue bug was due to the lack of an implemented copy constructor in one of the authors classes. The code looked a bit like this:

AClass.cpp:
class AClass
  {
  private:
    int*    m_pData;
  public:
    AClass()
    {
    m_pData = new int[10];
    }
    ~AClass()
    {
    delete [] m_pData;
    }
  void AddVals(int nVal1, int nVal2, int nVal3);
  };

SomeFile.cpp:
AClass GetVals()
  {
  AClass aObj;
  aObj.AddVals(1, 5, 7);
  return aObj;
  }

...

AClass aVals = GetVals();

… which doesn’t look too harmless (apart from the return by value). It wasn’t until I took a look in AClass.cpp and noticed that memory was been allocated in the constructor. See the potential problem yet? Line 4 of SomeFile.cpp constructs a new object of AClass and the AClass constructor invokes an allocation of memory. Line 6 returns the newly constructed aObj by value and invokes the default copy constructor (not operator=) on aVals, shallow copying the data from aObj into aVals. aObj is now out of scope and the destructor gets called on aObj, freeing the memory within. However, because m_pData within aObj was just shallow copied to aVals, aVals::m_pData still points to same memory location that aObj::m_pData pointed to.. and it’s just been free’d! So now aVals::m_pData points to invalid memory and any subsequent usage of said pointer will likely result in a crash immediately if you’re lucky, or n lines later if you aren’t (like me).

So what’s the point of all this rambling? Obey the Gang of Three! What’s the Gang of Three you ask? Destructor, Copy Constructor and Assignment Operator. If you need one of these three, you need them ALL. Even if you don’t ever intend to copy a class or do something silly like the example I posted above, no doubt someone will. If you don’t want people to make a copy of your objects, just make the copy constructor and assignment operator private.

Wondering exactly how to implement the Gang of Three? Here’s AClass from above expanded to include these functions:

AClass.cpp:
class AClass
  {
  private:
    int*    m_pData;

  public:
    AClass()
    {
    m_pData = new int[10];
    }

    ~AClass()
    {
    delete [] m_pData;
    }

    AClass(const AClass& copy)                 // Copy constructor
    {
    int* pMem = new int[10];  // Allocate memory
    // Perform deep copy e.g:
    for(int nIndex = 0; nIndex < 10; nIndex++)
        pMem[nIndex] = copy.m_pData[nIndex];

    m_pData = pMem;           // Assign new memory to m_pData
    }

    AClass& operator=(const AClass& rhs)       // Assignment operator
    {
    if(this != &rhs) // Don't want to self assign!
      {
      int* pNewMem = new int[10];   // Allocate new memory
      // Deep copy memory

      delete [] m_pData;            // Delete old data
      m_pData = pNewMem;            // Assign new memory to m_pData
      }
    return *this;
    }
  void AddVals(int nVal1, int nVal2, int nVal3);
  };

So to summarise: if you need to implement a destructor 99% of the time you should always ALWAYS ALWAYS implement a copy constructor and assignment operator lest you be cast down to memory hell!