Search code examples
c++memory-leaksoperator-overloadingcopy-constructorassignment-operator

C++ - overloaded assignment operator memory leaks


I have a class method that works with a copy of an object (*this, to be exact). The leaks occur within the overloaded assignment operator - that's what Visual Leak Detector says, anyway. What I'm doing is working with copy and if the work done is satisfactory I copy that newly created object back. I've also implemented a custom destructor, copy constructor and assignment operator because the problem occurs with dynamically allocated memory, obviously. My experience with C++ is quite limited so there could be some evil stuff in the code.

I will provide more info if needed.

Problematic method:

bool Grid::SurroundShipSquares(int top, int bottom, int left, int right)
{
    // copying itself
    Grid gridCopy(*this);
    Square** squaresCopy = gridCopy.GetSquares();
    for (int i = top; i <= bottom; ++i)
    {
        for (int j = left; j <= right; ++j)
        {
            if (squaresCopy[i][j].GetState() != SquareState::Vacant)
                return false;
            (squaresCopy[i][j]).SetState(SquareState::Unoccupiable);
        }
    }
    // the problem occurs here
    *this = gridCopy;
    return true;
}

Copy constructor:

Grid::Grid(const Grid& source)
{
    _position = source._position;
    _size = source._size;
    int dimensions = static_cast<int>(_size);
    _squares = new Square*[dimensions];
    for (int i = 0; i < dimensions; ++i)
    {
        _squares[i] = new Square[dimensions];
        for (int j = 0; j < dimensions; ++j)
        {
            _squares[i][j] = source._squares[i][j];
        }
    }
}

Assignment operator:

Grid& Grid::operator=(const Grid& source)
{
    if (this == &source) 
        return *this;
    _position = source._position;
    _size = source._size;
    int dimensions = static_cast<int>(_size);
    _squares = new Square*[dimensions];
    for (int i = 0; i < dimensions; ++i)
    {
        _squares[i] = new Square[dimensions];
        for (int j = 0; j < dimensions; ++j)
        {
            _squares[i][j] = source._squares[i][j];
        }
    }
    return *this;
}

Destructor:

Grid::~Grid()
{
    int dimensions = static_cast<int>(_size);
    for (int i = 0; i < dimensions; ++i)
    {
        delete[] _squares[i];
    }
    delete[] _squares;
}

Solution

  • The problem with your code is that you manage all your resources manually. This is terribly unsafe and a massive headache to do correctly, as is aptly demonstrated by both the existing answers being wrong.

    Use std::vector. This class will automatically manage all the memory for you, freeing you from having to do it yourself. This will greatly simplify your code, as well as making it correct.

    Also, self-assignment checking is an ancient anti-pattern. Do not include a check for self-assignment. If your assignment operator (you shouldn't really need to write your own with std::vector-based memory management in most cases) cannot handle self-assignment without a special case, it is broken.