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

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 2

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 3

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 4

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 5

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 6

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!
    }
}