Order of destructors

Question

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 ?

It's a bad sign when just a few lines into a class definition there's indication of [three star programming](http://wiki.c2.com/?ThreeStarProgrammer). — StoryTeller - Unslander Monica, Mar 04 '18 at 10:45
Please provide [mcve]. 1) For example: If `m_piece` was never initialized - it contains indeterminate value, and if you try dereferencing it (e.g. via the use of `delete`), your program exhibits undefined behavior. 2) We can see that you create an 2D array `m_board_fields`, which stores pointers to `BoardField`. Where are the contents of such array initialized? Since destructor tries to delete those, and, once again, dereferencing uninitialized pointers is undefined behavior. — Algirdas Preidžius, Mar 04 '18 at 10:45
I have coded C and C++ since 1992, but I never use three star pointers. The syntax is just too horrible and error prone. — Erik Alapää, Mar 04 '18 at 10:46
@StoryTeller the class definition is much larger, just didn't want to put everything here — Faustas Butkus, Mar 04 '18 at 10:46
Why don't you just use `std::array` with some helper functions implementing indexing instead of performing large amount of heap allocations? — user7860670, Mar 04 '18 at 10:48
Do you have a copy constructor implemented? An assignment operator? — StoryTeller - Unslander Monica, Mar 04 '18 at 10:50
Use `std::vector` or `std::array`. You are deleting but not allocating `m_board_fields[i][j]`. Also pay attention to things like `for (int j = 0; i < 8; j++)`. — n. m. could be an AI, Mar 04 '18 at 10:52

score 1 · Accepted Answer · answered Mar 04 '18 at 12:11

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.

Order of destructors

1 Answers1