C++ Destructor is being called when I don't expect it to be

Question

I added a destructor to some code and it seems to be calling early and causing problems. I added a debug statement to see where it is being called and that made me even more confused. I understand that managing my own memory is not best practice but I wanted to try it out myself.

This is basically my GameObject class:

class GameObject
{
public:
int             xCoord = 0, yCoord = 0, prevX, prevY;
int             status = 0, weight = -1;
int             id = -1;

GameObject(CommandComponent* commands,
    PhysicsComponent* physics,
    GraphicsComponent* graphics)
    : commandComponent(commands),
    physicsComponent(physics),
    graphicsComponent(graphics)
{};

~GameObject()
{
    std::cout << "Destructor called " << id << std::endl;
    delete commandComponent;
    delete physicsComponent;
    delete graphicsComponent;
};

void update(World& world, int command, sf::Time dt)
{
    commandComponent->update(*this, world, command);
    physicsComponent->update(*this, world);
    graphicsComponent->update(*this, dt);
};

void update(World& world, int command)
{
    commandComponent->update(*this, world, command);
    physicsComponent->update(*this, world);
};

sf::Sprite draw()
{
    return *(graphicsComponent->draw());
};

void setCoords(int x, int y)
{
    prevX = xCoord;
    xCoord = x;
    prevY = yCoord;
    yCoord = y;
};

void setId(int newId)
{
    id = newId;
}


private:
    CommandComponent*           commandComponent    = NULL;
    GraphicsComponent*          graphicsComponent   = NULL;
    PhysicsComponent*           physicsComponent    = NULL;

};

This is the createPlayer Method:

    GameObject* createPlayer(sf::Texture* text)
{
    return new GameObject(new PlayerCommandComponent(), new PlayerPhysicsComponent(), new PlayerGraphicsComponent(text));
};

This is a method I invoke to add the new object to a vector based on if it is an active object or an inactive one I also add it to an array :

void World::addObject(GameObject object, int id, int type){
object.setId(id);

if (type == 0)
{
    inactiveObjects.push_back(object);
}
else if (type == 1)
{
    activeObjects.push_back(object);
}
}

Finally this is my test code that creates the Game Objects and calls the above function and where I see the destructors being called from:

void World::test()
{
// Player
std::cout << "Starting to create id 0\n";
addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 0, 1);
activeObjects.at(0).setCoords(3, 3);
activeObjects.at(0).weight = 10;
std::cout << "Created id 0\n";

// Test Objects
std::cout << "Starting to create id 1\n";
addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 1, 1);
activeObjects.at(1).setCoords(3, 4);
activeObjects.at(1).weight = 7;
std::cout << "Created id 1\n";

addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 2, 1);
activeObjects.at(2).setCoords(5, 4);
activeObjects.at(2).weight = 2;

addObject((*createPlayer(&(mTextures.get(Textures::Enemy)))), 3, 1);
activeObjects.at(3).setCoords(6, 6);
activeObjects.at(3).weight = -1;

addObject((*createPlayer(&(mTextures.get(Textures::Enemy)))), 4, 1);
activeObjects.at(4).setCoords(1, 1);
activeObjects.at(4).weight = 0;

std::cout << "Done Creating Test Objects\n";

I guess my main question is how come the Destructors are being called? Im assuming its related to how I'm constructing the object in the createPlayer method, Perhaps it's going out of scope after I return it but I thought using the new keyword would prevent that from happening? I'm puzzled here.

Add this to your class: `GameObject(GameObject const&) = delete;` — Eljay, Jun 24 '19 at 23:14
An addendum to the above: @Eljay is showing you how to find accidental copies of your class. You can consider this an introduction to [What is the Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — user4581301, Jun 24 '19 at 23:17
Please never post screenshots of text - post the text itself. Also, you really need to learn about and use smart pointers instead of manually deallocating your memory. — xaxxon, Jun 24 '19 at 23:20
This is why you don't start to do manual management until you know what all of the special member functions do and when they're invoked. — Hatted Rooster, Jun 24 '19 at 23:23

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

Several things at play here.

GameObject* createPlayer(sf::Texture* text)

returns a dynamically allocated GameObject. This could be done better, read up on std::unique_ptr, but there is nothing strictly wrong here. I mention it mostly to point out std::unique_ptr and set up

addObject((*createPlayer(&(mTextures.get(Textures::PlayerCharacter)))), 0, 1);

because this is where thing start to go wrong. When you find code that uses new and dereferences and discards the the result, you're looking at a memory leak. You've lost the pointer to the dynamically allocated object and without the pointer it is next to impossible to find the allocation again so that you can delete it.

Storing the dereferenced object will invoke either the copy constructor or the assignment operator and at this point you need to consider The Rule of Three: If you need a to define a custom destructor, you probably need to define a custom assignment operator and a copy constructor. This is a standard example of when you need to observe the Rule of Three. What goes wrong is well-explained in the Rule of Three Link, so stop, read, and understand it before going any further. Failure to do this means the rest of this answer will be nigh-useless to you.

You cannot write good, non-trivial C++ code without a firm grip on the Rule of Three and all of its friends.

You can step around the Rule of Three here by changing

void World::addObject(GameObject object, int id, int type)

to

void World::addObject(GameObject * object, int id, int type)

and pass object by reference. This doesn't help much because

inactiveObjects.push_back(object);

is expecting an object, not a pointer.

You can change that as well, but should you? std::vector is at its absolute best when it directly contains an object. Pointers lead to pointer chasing, poor caching behaviour and ultimately suuuhhhfering. Don't store pointers unless you have a compelling reason to do so.

And if you do, manage the pointers with a std::unique_ptr beginning to end.

What I would do:

Jump straight over the Rule of Three and go to The Rule of Five.

Exterminate as many dynamically allocated variables as possible so that I don't need to do much work, if any, with point 2. This means no pointers for (or in) commandComponent, physicsComponent and graphicsComponent if possible.
Add a move constructor and move assignment operator to GameObject as well as CommandComponent, PhysicsComponent, and GraphicsComponent. Keep all resource management as close to the resource as possible. This allows you to keep higher level classes as ignorant as possible. If GraphicsComponent knows how to copy and move itself, GameObject doesn't need to know how to move it. This allows you to take advantage of The Rule of Zero, and the Rule of Zero should be what you strive for in all of your classes.
Use move semantics to get a GameObject, not a GameObject* from createPlayer down to the activeObjects and inactiveObjects vectors.
Enjoy the reduced memory management load.

Thank you for all of your help I think I understand what is happening now. I am allocating memory in the call to create the new Game Object, when it is returning the new GameObject it calls the copy constructor but since I didn't specify a copy constructor it essentially is just performing a shallow copy of the data including a copy of the pointer to the allocated memory. But the new instance of the pointer is pointing to the created Object's memory that is then de-allocated in the destructor since it goes out of scope. — Cody Savage, Jun 25 '19 at 19:47
@CodySavage It's not the return of the `GameObject` from `createPlayer` because that's a pointer. The (first) copy is done when you take that pointer, dereference it, and pass it by value into `addObject`. More copies could follow placing the copy into the `vector` and any time the `vector` resizes, shuffles, removes, .... It's minor compared to the copying problem, but don't forget the memory leak you get from discarding the pointer without `delete`ing it. The unexpected destructions occur whenever any of the copies go out of scope. — user4581301, Jun 25 '19 at 20:02

C++ Destructor is being called when I don't expect it to be

1 Answers1

What I would do: