Should objects delete themselves in C++?

Question

I've spent the last 4 years in C# so I'm interested in current best practices and common design patterns in C++. Consider the following partial example:

class World
{
public:
    void Add(Object *object);
    void Remove(Object *object);
    void Update();
}

class Fire : Object
{
public:
    virtual void Update()
    {
        if(age > burnTime)
        {
            world.Remove(this);
            delete this;
        }
    }
}

Here we have a world responsible for managing a set of objects and updating them regularly. Fire is an an object that might be added to the world under many different circumstances but typically by another object already in the world. Fire is the only object that knows when it has burned out so currently I have it deleting itself. The object that created the fire is likely no longer in existence or relevant.

Is this a sensible thing to do or is there a better design that would help clean up these objects?

Answer 1

I don't this there's anything inherently wrong with an object deleting itself, but another possible method would be to have the world be responsible for deleting objects as part of ::Remove (assuming all Removed objects were also deleted.)

Answer 2

There is nothing really wrong with objects deleting themselves in C++, most implementations of reference counting will use something similar. However, it is looked on as a slightly esoteric technique, and you will need to keep a really close eye on the ownership issues.

Answer 3

It's not generally a good idea to do a "delete this" unless necessary or used in a very straightforward way. In your case it looks like the World can just delete the object when it is removed, unless there are other dependencies we're not seeing (in which case your delete call will cause errors since they aren't notified). Keeping ownership outside your object allows your object to be better encapsulated and more stable: the object won't become invalid at some point simply because it chose to delete itself.

Answer 4

I prefer a stricter ownership model. The world should own the objects in it and be responsible for cleaning them up. Either have world remove do that or have update (or another function) return a value that indicates that an object should be deleted.

I am also guessing that in this type of pattern, you are going to want to reference count your objects to avoid ending up with dangling pointers.

Answer 5

That's a fairly common reference counting implementation and I've used that successfully before.

However, I've also seen it crash. I wish I could remember the circumstances for the crash. @abelenky is one place I have seen it crash.

It might have been where you further subclass Fire, but fail to create a virtual destructor (see below). When you don't have a virtual destructor, the Update() function will call ~Fire() instead of the appropriate destructor ~Flame().

class Fire : Object
{
public:
    virtual void Update()
    {
        if(age > burnTime)
        {
            world.Remove(this);
            delete this; //Make sure this is a virtual destructor.
        }
    }
};

class Flame: public Fire
{
private: 
    int somevariable;
};

Answer 6

The delete will fail and may crash the program if the object was not allocated and constructed with new. This construct will prevent you from instantiating the class on the stack or statically. In your case, you appear to be using some kind of allocation scheme. If you stick to that, this might be safe. I certainly wouldn't do it, though.

Answer 7

You have made Update a virtual function, suggesting that derived classes may override the implementation of Update. This introduces two big risks.

1.) An overridden implementation may remember to do a World.Remove, but may forget the "delete this". The memory is leaked.

2.) The overridden implementation calls the base-class Update, which does a "delete this", but then proceeds with more work, but with an invalid this-pointer.

Consider this example:

class WizardsFire: public Fire
{
public:
    virtual void Update()
    {
        Fire::Update();  // May delete the this-object!
        RegenerateMana(this.ManaCost); // this is now invaild!  GPF!
    }
}

Answer 8

It certainly works for objects created by new and when the caller of Update is properly informed about that behavior. But i would avoid it. In your case, the ownership clearly is at the World, so i would make the world delete it. The object does not create itself, i think it should also not delete itself. Can be very surprising if you call a function "Update" on your object, but then suddenly that object is not existing anymore without World doing anything out of itself (apart from Removing it out of its list - but in another frame! The code calling Update on the object will not notice that).

Some ideas on this

1. Add a list of object references to World. Each object in that list is pending for removal. This is a common technique, and is used in wxWidgets for toplevel windows that were closed, but may still receive messages. In idle time, when all messages are processed, the pending list is processed, and objects are deleted. I believe Qt follows a similar technique.
2. Tell the world that the object wants to be deleted. The world will be properly informed and will take care of any stuff that needs to be done. Something like a deleteObject(Object&) maybe.
3. Add a shouldBeDeleted function to each object which returns true if the object wishes to be deleted by its owner.

I would prefer option 3. The world would call Update. And after that, it looks whether the object should be deleted, and can do so - or if it wishes, it remembers that fact by adding that object to a pending-removal list manually.

It's a pain in the ass when you can't be sure when and when not to be able to access functions and data of the object. For example, in wxWidgets, there is a wxThread class which can operate in two modes. One of these modes (called "detachable") is that if its main function returns (and the thread resources should be released), it deletes itself (to release the memory occupied by the wxThread object) instead of waiting for the owner of the thread object to call a wait or join function. However, this causes severe headache. You can never call any functions on it because it could have been terminated at any circumstances, and you can not have it created not with new. Quite some people told me they very much dislike that behavior of it.

The self deletion of reference counted object is smelling, imho. Let's compare:

// bitmap owns the data. Bitmap keeps a pointer to BitmapData, which
// is shared by multiple Bitmap instances. 
class Bitmap {
    ~Bitmap() {
        if(bmp->dec_refcount() == 0) {
            // count drops to zero => remove 
            // ref-counted object. 
            delete bmp;
        }
    }
    BitmapData *bmp;
};

class BitmapData {
    int dec_refcount();
    int inc_refcount();

};

Compare that with self-deleting refcounted objects:

class Bitmap {
    ~Bitmap() {
        bmp->dec_refcount();
    }
    BitmapData *bmp;
};

class BitmapData {
    int dec_refcount() {
        int newCount = --count;
        if(newCount == 0) {
            delete this;
        }
        return newCount;
    }
    int inc_refcount();
};

I think the first is so much nicer, and i believe well designed reference counted objects do not do "delete this", because it increases coupling: The class using the reference counted data has to know and remember about that the data deletes itself as a side-effect of decrementing the reference-count. Note how "bmp" becomes possibly a dangling pointer in ~Bitmap's destructor. Arguably, not doing that "delete this" is much nicer here.

Answer to a similar question "What is the use of delete this"

Answer 9

The problem with this is that you're really creating an implicit coupling between the object and the World class.

If I try to call Update() outside the World class, what happens? I might end up with the object being deleted, and I don't know why. It seems the responsibilities are badly mixed up. This is going to cause problems the moment you use the Fire class in a new situation you hadn't thought of when you wrote this code. What happens if the object should be deleted from more than one place? Perhaps it should be removed both from the world, the current map, and the player's inventory? Your Update function will remove it from the world, and then delete the object, and the next time the map or the inventory tries to access the object, Bad Things Happen.

In general, I'd say it is very unintuitive for an Update() function to delete the object it is updating. I'd also say it's unintuitive for an object to delete itself. The object should more likely have some kind of way to fire an event saying that it has finished burning, and anyone interested can now act on that. For example by removing it from the world. For deleting it, think in terms of ownership.

Who owns the object? The world? That means the world alone gets to decide when the object dies. That's fine as long as the world's reference to the object is going to outlast an other references to it. Or does the object own itself? What does that even mean? The object should be deleted when the object no longer exists? Doesn't make sense.

But if there is no clearly defined single owner, implement shared ownership, for example using a smart pointer implementing reference counting, such as boost::shared_ptr

But having a member function on the object itself, which is hardcoded to remove the object from one specific list, whether or not it exists there, and whether or not it also exists in any other list, and also delete the object itself regardless of which references to it exist, is a bad idea.

Answer 10

delete this is very risky. THere's nothing "wrong" with it. It's legal. But there are so many things that can go wrong when you use it that it should be used sparingly.

Consider the case where someone has open pointers or references to the object, and then it goes and deletes itself.

In most cases, i think the object lifetime should be managed by the same object that creates it. It just makes it easier to know where destruction occurs.

Answer 11

Others have mentioned problems with "delete this." In short, since "World" manages the creation of Fire, it should also manage its deletion.

One other problem is if the world ends with a fire still burning. If this is the only mechanism through which a fire can be destroyed, then you could end up with an orphan or garbage fire.

For the semantics you want, I would have an "active" or "alive" flag in your "Fire" class (or Object if applicable). Then the world would on occasion check its inventory of objects, and get rid of ones that are no longer active.

--

One more note is that your code as written has Fire privately inheriting from Object, since that is the default, even though it is much less common than public inheiritance. You should probably make the kind of inheritance explicit, and I suspect you really want public inheritiance.