How to stop this pointer memory-leak?

Question

I'm decent at c++ but I've always been bad when it comes to pointers and memory. I've gotten into this situation where I don't know if there is a solution.

typedef unsigned long long ullong;

class MathClass { //This is just an example class
public:
    MathClass() {num = new ullong[1]();}

    MathClass operator+(MathClass b) { //This is not my actual function, just one that has the same problem
        MathClass c;
        c.num[0] = num[0] + b.num[0];
        delete [] num;
        num = NULL;
        return c;
    }
public:
    ullong* num;
};

This would work for a situation like this.

MathClass a;
MathClass b;
for (int i = 0; i < 1000; i++) {
    a = a + b;
}

Because I'm setting a equal to a + b, so when the + function is run it would set a equal to c and delete the old a num.

For a situation like this it would cause an error because I'm deleting b's num.

MathClass a;
MathClass b;
MathClass c;
for (int i = 0; i < 1000; i++) {
    a = b + c;
}

If I did not delete num this would work, but that causes memory leaks. The memory for this easily goes over 100MB when I don't delete num. I'm sure the answer to this is simple but I can't figure it out.

Answer 1

You need the rule of three (five).

When you allocate memory, or if you need to implement assignment, destructor, or copy constructor, then you need all three (five - move constructor and move assignment).

When you allocate the memory with new (consider shared_ptr, unique_ptr) you require to control how copy assignment and delete work.

class MathClass { //This is just an example class
public:
    MathClass() {num = new ullong[1]();}
    ~MathClass() { delete [] num;} // cleans up memory.
    MathClass( const MathClass & rhs ) { num = new ullong[1](); num[0] = rhs.num[0]; }
    MathClass& operator=( const MathClass & rhs )
    {
       if( &rhs != this ) {
          num[0] = rhs.num[0];
       }
       return *this;
    }

    MathClass operator+(MathClass b) { //This is not my actual function, just one that has the same problem
    MathClass c;
    c.num[0] = num[0] + b.num[0];
    // the wrong place delete [] num;
    num = NULL;
    return c;
    }
public:
    ullong* num;
};

The delete [] in the operator+ was in the wrong place, as it was trying to find the correct place to free the memory. However, the easiest way of getting it all to work is to apply the rule of 3 and ensure that memory was constructed at construction, deleted at deletion, and the assignment (and move) operators work correctly

Answer 2

the issue actually is not exactly in the pointers but operator overloading and class implementation ( as it was mentioned in comments ). If you chage argumetnts order in your first example ( i.e. a = a + b -> a = b + a ) you will see the same error as in the second example. So here is good article on implementing operator overloading The code could look like this

#include <iostream>
#include <algorithm>

typedef unsigned long long ullong;

class MathClass {

public:
    MathClass() { num = new ullong[ 1 ](); }
    MathClass( const MathClass &a ) { 
        *this = a;
    }
    ~MathClass() {  
        delete[] num;
        num = NULL;
    }

    MathClass &operator=( const MathClass &a ) {
        if ( this != &a ) {
            num = NULL;
            num = new ullong[ 1 ];
        }
        std::copy( a.num, a.num + 1, num );

        return *this;
    }

    friend MathClass operator+( const MathClass &a, const MathClass b ) {
        MathClass c;
        c.num[ 0 ] = a.num[ 0 ] + b.num[ 0 ];

        return c;

    }
ullong *num;
};

int main( int argc, char **argv ) {
   MathClass a;
   MathClass b;
   MathClass c;
   for ( int i = 0; i < 1000; ++i ) {
      std::cout << "a.num[ 0 ] is " << a.num[ 0 ] << std::endl;
      std::cout << "b.num[ 0 ] is " << b.num[ 0 ] << std::endl;
      a = b + a;
      a = c + b;
   }

   return 0;
}

In main "std::cout" used only for output visibility. There are certainly much better implementation of copy constructor then mine but the key point here is that when you are using coplex types you nearly always have to reimplement copy and assigns operator as they are very often ( if not always ) are called. There are some other moments that can be good to mention about your code. At least since C++11 NULL changed to nullptr, typedef is also not so common. Use reference instead of pointers and it is good practice especially in case of complex arguments to pass them by reference, not by value as in case of passing by value copy constructor will be called and you will have to instance of the same argument which can lead to memory overuse.