C++ Destructor is not called

Question

I've got three classes: Room, Door and World

#include <set>;

using namespace std;

class Door; // Forward declaration

class Room {
public:
    Door* door1;
    Door* door2;

    Room(){}

    ~Room() {
        delete door1;
        door1 = 0;
        delete door2;
        door2 = 0;
    }
};

class Door {
public:
    Room* roomA;
    Room* roomB;

    Door(Room* roomA, Room* roomB) {
        this->roomA = roomA;
        this->roomB = roomB;
        linkRooms(); // This sets up the Door-Pointers in the Rooms
                     // so they know about the new door.
    }    

    ~Door() {
        // Returns the room-owned pointer pointing at this door
        getMyRoomPointer(roomA) = 0;
        getMyRoomPointer(roomB) = 0;
    }    

    Door * & getMyRoomPointer(Room * const & room) {
        if (room->door1 == this) return room->door1;
        else return room->door2;
    }

    void linkRooms() {
        roomA->door1 = this;
        roomB->door2 = this;
    }

};

class World {
public:
    std::set<Room*> rooms;

    World() {
        // Set up two rooms and link them using a door
        Room* newRoom = new Room();
        rooms.insert(newRoom);
        Room* anotherNewRoom = new Room();
        rooms.insert(anotherNewRoom);

        new Door(newRoom, anotherNewRoom);
    }
    ~World() {
        // Iterate over the rooms and call delete on all of them
        for (std::set<Room*>::iterator it = rooms.begin(); it != rooms.end(); ++it) {
            delete *it;
        }
    }
};

int main() {

    World world;

    return 0;
}

When running main, the constructor fills the world with just two rooms and a door as a link between them. After main returns, world should be deleted and all the rooms and doors also should be taken care of.

The thing is, that my Door destructor is never called. So the Door pointers inside the rooms are not set to null and I get an error when the room "at the other side" tries to delete the same door.

When I just create an instance of Door, and than delete it right afterwards, I'm not facing any problems:

int main(){

    Room oneRoom;
    Room anotherRoom;

    Door* door = new Door(&oneRoom, &anotherRoom);
    delete door; // works just fine

    return 0;
}

Questions: Why is the Door constructor not called? Is setting up Door like in the first example allowed?

I know, that I'm double deleting my room's Door pointers, and that I could (and should) use SmartPointers. Right now I'm just wondering why I am facing that kind of behavior. I'm still new to C++, after all.

I did set up a runnable example now, that reproduces the error.

Answer 1

You should pay attention to compiler warning messages:

g++ -std=c++17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++      41120443.cpp    -o 41120443
41120443.cpp:1:15: warning: extra tokens at end of #include directive
 #include <set>;
               ^
41120443.cpp:7:7: warning: ‘class Room’ has pointer data members [-Weffc++]
 class Room {
       ^~~~
41120443.cpp:7:7: warning:   but does not override ‘Room(const Room&)’ [-Weffc++]
41120443.cpp:7:7: warning:   or ‘operator=(const Room&)’ [-Weffc++]
41120443.cpp: In constructor ‘Room::Room()’:
41120443.cpp:12:5: warning: ‘Room::door1’ should be initialized in the member initialization list [-Weffc++]
     Room(){}
     ^~~~
41120443.cpp:12:5: warning: ‘Room::door2’ should be initialized in the member initialization list [-Weffc++]
41120443.cpp: In destructor ‘Room::~Room()’:
41120443.cpp:15:16: warning: possible problem detected in invocation of delete operator: [-Wdelete-incomplete]
         delete door1;
                ^~~~~
41120443.cpp:15:16: warning: invalid use of incomplete type ‘class Door’
41120443.cpp:5:7: note: forward declaration of ‘class Door’
 class Door; // Forward declaration
       ^~~~
41120443.cpp:15:16: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined
         delete door1;
                ^~~~~
41120443.cpp:17:16: warning: possible problem detected in invocation of delete operator: [-Wdelete-incomplete]
         delete door2;
                ^~~~~
41120443.cpp:17:16: warning: invalid use of incomplete type ‘class Door’
41120443.cpp:5:7: note: forward declaration of ‘class Door’
 class Door; // Forward declaration
       ^~~~
41120443.cpp:17:16: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined
         delete door2;
                ^~~~~
41120443.cpp: At global scope:
41120443.cpp:22:7: warning: ‘class Door’ has pointer data members [-Weffc++]
 class Door {
       ^~~~
41120443.cpp:22:7: warning:   but does not override ‘Door(const Door&)’ [-Weffc++]
41120443.cpp:22:7: warning:   or ‘operator=(const Door&)’ [-Weffc++]
41120443.cpp: In constructor ‘Door::Door(Room*, Room*)’:
41120443.cpp:27:5: warning: ‘Door::roomA’ should be initialized in the member initialization list [-Weffc++]
     Door(Room* roomA, Room* roomB) {
     ^~~~
41120443.cpp:27:5: warning: ‘Door::roomB’ should be initialized in the member initialization list [-Weffc++]
41120443.cpp: In constructor ‘World::World()’:
41120443.cpp:56:5: warning: ‘World::rooms’ should be initialized in the member initialization list [-Weffc++]
     World() {
     ^~~~~

and Valgrind output:

valgrind  ./41120443 
==2864== Memcheck, a memory error detector
==2864== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==2864== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==2864== Command: ./41120443
==2864== 
==2864== Conditional jump or move depends on uninitialised value(s)
==2864==    at 0x4C2C291: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2864==    by 0x108BC3: Room::~Room() (41120443.cpp:17)
==2864==    by 0x108D67: World::~World() (41120443.cpp:68)
==2864==    by 0x108B65: main (41120443.cpp:75)
==2864== 
==2864== Conditional jump or move depends on uninitialised value(s)
==2864==    at 0x4C2C291: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2864==    by 0x108BA8: Room::~Room() (41120443.cpp:15)
==2864==    by 0x108D67: World::~World() (41120443.cpp:68)
==2864==    by 0x108B65: main (41120443.cpp:75)
==2864== 
==2864== Invalid free() / delete / delete[] / realloc()
==2864==    at 0x4C2C2DB: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2864==    by 0x108BC3: Room::~Room() (41120443.cpp:17)
==2864==    by 0x108D67: World::~World() (41120443.cpp:68)
==2864==    by 0x108B65: main (41120443.cpp:75)
==2864==  Address 0x5a82e00 is 0 bytes inside a block of size 16 free'd
==2864==    at 0x4C2C2DB: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2864==    by 0x108BA8: Room::~Room() (41120443.cpp:15)
==2864==    by 0x108D67: World::~World() (41120443.cpp:68)
==2864==    by 0x108B65: main (41120443.cpp:75)
==2864==  Block was alloc'd at
==2864==    at 0x4C2B21F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2864==    by 0x108CCE: World::World() (41120443.cpp:63)
==2864==    by 0x108B54: main (41120443.cpp:75)

You could make life much easier for yourself with judicious use of smart pointers:

#include <memory>
#include <set>

class Door; // Forward declaration

struct Room {
    std::shared_ptr<Door> door1 = {};
    std::shared_ptr<Door> door2 = {};

    ~Room();
};

struct Door {
    const std::weak_ptr<Room> roomA;
    const std::weak_ptr<Room> roomB;

    Door(std::shared_ptr<Room> roomA, std::shared_ptr<Room> roomB)
        : roomA(roomA),
          roomB(roomB)
    {
        roomA->door1 = roomB->door1 = std::shared_ptr<Door>{this};
    }

    ~Door() = default;
};

// Now that Door is complete, we can define ~Room
Room::~Room() = default;

struct World {
    std::set<std::shared_ptr<Room>> rooms = {};

    World() {
        // Set up two rooms and link them using a door
        auto newRoom = std::make_shared<Room>();
        rooms.insert(newRoom);
        auto anotherNewRoom = std::make_shared<Room>();
        rooms.insert(anotherNewRoom);

        new Door(newRoom, anotherNewRoom);
    }

    ~World() = default;
};

int main() {

    World world;

    return 0;
}

This builds and runs cleanly:

g++ -std=c++17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++      41120443.cpp    -o 41120443
valgrind  ./41120443 
==3254== Memcheck, a memory error detector
==3254== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==3254== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==3254== Command: ./41120443
==3254== 
==3254== 
==3254== HEAP SUMMARY:
==3254==     in use at exit: 0 bytes in 0 blocks
==3254==   total heap usage: 7 allocs, 7 frees, 72,952 bytes allocated
==3254== 
==3254== All heap blocks were freed -- no leaks are possible
==3254== 
==3254== For counts of detected and suppressed errors, rerun with: -v
==3254== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Answer 2

You never delete the door

new Door(newRoom, anotherNewRoom);

You should add Door *door in world

public:
  Door *door;

...

  door = new Door(newRoom, anotherNewRoom);

and call delete in the destructor of world:

delete door;

note that your code has a lot of undefined behavior.

---

std::set<Room *> rooms;

std::set is an associative container that contains a sorted set of unique objects of type Key.

You should use list:

std::list<Room *> rooms;

Answer 3

You call delete before Door has been defined. Therefore the behaviour of the program is undefined, and the destructor is not guaranteed to be called.

A quote from the standard (draft) [expr.delete]:

5. If the object being deleted has incomplete class type at the point of deletion and the complete class has a non-trivial destructor or a deallocation function, the behavior is undefined.

Solution: If the destructor of the type is non-trivial (i.e. user defined, such as the ~Door, then never delete such object until the type is complete). In this case, define Door, before the functions that call delete.

As a general rule, you can never call a member function unless the class type is complete. Unfortunately in the case of destructor, it may not always be possible for the compiler to catch the bug. PS. g++ does warn about your program: warning: possible problem detected in invocation of delete operator: [-Wdelete-incomplete]