Search code examples
c++destructorobject-lifetime

Order of destructors


I have these kind of classes:

Game:

class Game {
private:
    BoardField*** m_board_fields;
public:
    Game() { 
        m_board_fields = new BoardField**[8];
        for (int i = 0; i < 8; i++) {
             m_board_fields[i] = new BoardField*[8]; 
        }
    }

    Game::~Game() {
        for (int i = 0; i < 8; i++) {
            for (int j = 0; i < 8; j++) {
                delete m_board_fields[i][j];
            }

            delete[] m_board_fields[i];
        }

        delete[] m_board_fields;
    }
}

BoardField:

class BoardField {
private:
    ChessPiece* m_piece;
    ....
public:
    BoardField::~BoardField() {
        delete m_piece;
    }
}

And on the close of the program I get error in ~BordField:

Exception thrown: read access violation. this was 0xFDFDFDFD.

Did I made my destructors incorrect? What is the best way to clear memory from multidimensional array ?


Solution

  • There is are two fundamental flaws in your design:

    • there is no clear ownership of the BoardFields: someone create it, someone else deletes it. It can work if you're very cautious but it's error prone.
    • you do not ensure the rule of 3 (or better 5): if you have any piece of code where you create a copy of either your Game or a of any BoardField the first object that gets destroyed will delete the m_piece pointer, and when the second object gets destroyed, it'll try to delete a second time the same pointer, which is UB.

    There is a third important issue: you're over-using raw pointers:

    • if m_board_fields is a 2d array of fixed size, make it a fixed size array (aka BoardField* m_board_fields[8][8]). If you want to keep its size dynamic, use vectors.
    • a cell of m_board_field could be a pointer if there's some polymorphism expected. But this seems not the case here, as obviously ChessPiece is the polymorphic class. So better use plain fields instead of pointers (aka BoardField m_board_fields[8][8]).
    • Finally, instead of using raw pointer to ChessPiece, better use a shared_ptr<ChessPiece> : you don't have to worry about shallow pointer copies and double delete; the shared_ptr will take care of itself and destroy the object if it's no longer used.