Class destruction segfault

Question

This post is not a duplicate. Read my question first.

I'm certainly missing something here. I would like to have my Entity class be able to destroy an instance of itself when the health quantity is zero.

    virtual int takeDamage(int damage)  {
    health -= damage;
    if(health <= 0) delete this;
}

What's wrong with the above code? The program crashes with a segfault when the last line above is called. So far I have tried:

• Making a custom destructor

• Making a destructor function:

  virtual void destroy(Entity*e) { delete e; }

• Destroying the object where it was allocated

I removed the last line of takeDamage() and then called delete object from main.

if(tris[i]->getHealth() <=0) delete tris[i]; //called from the main function where a temporary collision detection occurs over all the entities in the program

I've located some of the issue, and it comes down to the following. The instances of all the Entity(s) in my program (of one particular specialization, ie: Triangles) are stored in a vector. For example, the above code uses this vector: vector<Triangle*> tris; (Triangle inherits Entity). Then, that vector is iterated through and various operations are performed, such as collision detection, AI, etc. Now, when one of the objects is deleted, the next time we have iterated through the entire vector, we come to the object which has been deleted. The next question is, what would I do to shrink that vector? (Now this is a good place to flag the question is perhaps needing of a separate question!)

Answer 1

Given your description, There is a way to do this safely using algorithm functions. The algorithm functions you may be looking for are std::stable_partition, for_each, and erase.

Assume you have the "game loop", and you're testing for collisions during the loop:

#include <algorithm>
#include <vector>
//...
std::vector<Entity*> allEntities;
//...
while (game_is_stlll_going())
{
    auto it = allEntities.begin();
    while (it != allEntitities.end())
    {
        int damage = 0;
        // ... assume that damage has a value now.
        //...
        // call the takeDamage() function
        it->takeDamage(damage);
        ++it;
    }

    // Now partition off the items that have no health
    auto damaged = std::stable_partition(allEntities.begin(),    
           allEntities.end(), [](Entity* e) { return e->health > 0; });

    // call "delete" on each item that has no health
    std::for_each(damaged, allEntities.end(), [] (Entity *e) { delete e; });

    // erase the non-health items from the vector.
    allEntities.erase(damaged, allEntities.end());

    // continue the game...
} 

Basically what the loop does is that we go through all the entities and call takeDamage for each one. We don't delete any entities at this stage. When the loop is done, we check to see which items have no health by partitioning off the damaged items using the std::stable_partition algorithm function.

Why stable_partition and not just call std::remove or std::remove_if and before erasing the items, call delete on the removed items? The reason is that with remove / remove_if, you cannot do a multi-step deletion process (call delete and then erase it from the vector). The remove/remove_if algorithm functions assumes what has been moved to the end of the container (i.e. the "removed" items) are no longer needed and thus cannot be used for anything except for the final call to vector::erase. Calling delete on removed items is undefined behavior.

So to solve this issue, you need to "partition off" the bad items first, deallocate the memory for each item, and then remove them. We need to use a partitioning algorithm, so we have a choice of std::stable_partition or std::partition. Which one? We choose std::stable_partition. The reason why we chose std::stable_partition over std::partition is to keep the relative order of the items intact. The order of the items may be important for your game implementation.

Now we call stable_partition to partition the items into two sides of the entities vector -- the good items on the left of the partition, the bad items on the right of the partition. The lambda function is used to decide which entity goes where by testing the health value. The "partition" in this case is an iterator which is returned by stable_partition.

Given this, we call for_each, where the lambda calls delete on each item to the right of the partition, and then a vector::erase() to remove everything to the right of the partition. Then we loop again, redoing this whole process until the game is finished.

Another thing you will notice is the safety of the code above. If all entries have some health, then the calls to stable_partition, for_each, and erase are essentially no-ops. So there is no need to explicitly check for at least one item having no health (but nothing stops you to from doing so, if you feel you need this micro-optimization).

And also, make sure your Entity base class has a virtual destructor, otherwise your code will exhibit undefined behavior on the delete.

Edit: The takeDamage() function should be rewritten to not call delete this.

 virtual int takeDamage(int damage)  {
    health -= damage;
    return 0;
}