Bad form for an object to delete itself (C++)?
 

I've written a lib which contains some automatic cleanup code. It basically assembles objects similar to binary trees, and when a node gets deleted, it also deletes its children. The destructor is protected for the node object; you have to call a 'Destruct()' method, which checks to see if the node is a child of anything. If not, it calls 'delete this;' on itself. Is this bad form? Obviously you wouldn't use it on a stack allocated object, but that's as common sense as not trying to 'delete' (proper) a stack variable. So far it's worked perfectly; I've had no run time errors with it.
ta0kira

There's nothing inherently wrong with it, but it is a "surprising" API. It might be cleaner to have the removal of a child be a method of the parent node. i.e.
This might remove the need for the child to carry a pointer to its parent (I don't know how the rest of your code works of course).

If you sometime later decided that a child can have multiple parents (perhaps the same node object is used in multiple trees and reference counted), the same removeChild() API would still work (i.e., only deletes the child if its reference count reaches 0). The Destruct() method wouldn't work because it doesn't say which parent to unlink the child from.

That's a good idea, however it won't work with my implementation; the functions which remove or replace a node return a parentless node. This node is free to either Destruct() or to add to another tree somewhere. This makes rearranging trees simpler while still maintaining the inherent memory management aspect. I actually don't allow the user to call the method directly, either; I require them to call a static function using the pointer as an argument (makes it appear more like a 'delete' call, plus checks for NULL.)

If you'd like to see the code, here it is: http://sourceforge.net/projects/hparser-ta0kira/

Unfortunately I don't have it documented at this point. It does have a few UML pics, though. Documentation is my current forefront project.
ta0kira

"delete this" is a common idiom (especially in Microsoft-land) - and it's perfectly acceptable C++.

yeah I wouldn't sweat it too much. Sounds like things will work fine the way you have them.

I don't get why you can't just do this in the destructor. Does it really matter to the lib users what data structure you are using? After doing "delete this", you must be absolutely certain that you nor anybody else is going to try to do anything with the object, or even pointers to the object.

Also, I'm not sure that Microsoft-land idioms are necessary moral. Prime example is the kitchensink.h idiom, implemented as stdafx.h. That idiom breaks make and gcc dependency checking, causing builds to take much longer than they otherwise would.

The problem with ~ is that it doesn't return; 'delete' is a "directive" operation that has to throw an exception if it doesn't work. The Destruct() function returns NULL if successful, and 'this' if not. The function essentially forces the user to do a check for ownership prior to 'delete'. Were it left up to them, they might inadvertently delete something that something else intended to later.

I've gone to great lengths to ensure that the user can't easily change the ownership of nodes. Unless the user decides to store a pointer or poorly reimplement a virtual (as they can do with anything else), there should be no problems with bad pointers.
ta0kira

How can it fail exactly? Destructors simply don't fail, and if they possibly could, it is definitely exceptional. Since it sounds like all of the ownership issues for these nodes are supposed to be handled entirely by the library, what does the user have to check for? Reiterating a previous query, could you implement this library using an array, map or set, ignoring performance issues? What is this library for, anyway?

By making the destructor protected, it can't be a stack variable. That's the point of making it protected.

You can also get some Google hits on "C++ delete this", that might be interesting reading.


Also, I like tuxdev's idea of using what standard C++ gives, or even going one step farther and using what C++0x will give you by using the boost library's boost::shared_ptr, e.g.
The default node destructor and the boost::shared_ptr will handle most (all?) of the memory management you need.

The problem is that as the tree is assembled, parent nodes take ownership of their children via private std::auto_ptr. This causes the child to be deleted when the parent is, thereby causing a cascade that deletes an entire tree or branch if the base is deleted. If a user were to 'delete' an arbitrary node it may be the child of something, and hence owned by an auto_ptr that will expect to delete it. At that point it's a bad pointer; therefore, the auto_ptr causes a bad delete.

By removing a node or branch from a tree it becomes ownerless and can therefore safely be deleted; this is what Destruct() checks for. I've gone to a lot of trouble to ensure that owned status is correctly indicated.That's the point; by making the Destruct() function, the user doesn't have to worry about a check for ownership that they might not even be aware of when trying to delete a node. In all reality they should know if they are dealing with an independent branch or one that's attached to a tree, though.I can't implement this using an array or conventional container; I'd lose quite a bit of inherent functionality. The library provides a data tree infrastructure intended for file parsing. The "one-at-a-time" allocation allows for trees to be assembled as the file is read, and having each node managed by its parent (converging to a single base) allows for entire branches to be removed and placed on other trees or deleted entirely independently of context.The base class is abstract with a protected destructor. The structural management is in a virtually derived class, also with a protected destructor. The user implements this derived class, however unless they derive it virtually their object can be created on the stack. There's no stopping that. (Well, maybe; I'll let you know if my secret idea works...)
ta0kira

Okay, so you are worried that by deleting a child, a parent will have a dangling pointer. The two best ways to approach this in what I understand are your limitations is to either inform the parent somehow that it doesn't own the child anymore, or use a wrapper object that keeps track of whether or not the node is a root node, and does nothing if it isn't (with an assert, since it generally isn't supposed to happen, right?).

I could easily have the child inform its owner of its own deletion, however I find that to be counterintuitive; does it delete the whole branch, or remove that node and move the rest up? I'd rather force the user to be explicit by making them remove it (and have the branch obviously ownerless) before it can be deleted. It cuts down on unexpected results, and isn't intended to "veto" their delete, but to let them know ahead of time that they need to remove a node/branch from it's tree before the lib will delete it.

There's no need for a wrapper, either; it knows if it's owned by something else. So basically what you are suggesting is a preference for the destructor to assert upon deletion of an owned node vice a function with a return value which indicates the same? I like to avoid causing flagrant errors as much as I like to avoid getting them.

The other option is to put everything in the static function, although transparent to the user. This would prevent the user from overriding the Destruct() function, however, which they might need to do since the base class is entirely abstract.
ta0kira

I find that it is counter intuitive for the whole branch not to get deleted, except that it might cause problems with other parse trees that may be using it, and then just do reference counting.

The major problem I have with Destroy() is that there is a natural semi-equavalent in C++ already, and it is best to use the builtin stuff, even if you might have to go through a few hoops to do so. Since deleting a child node is a code problem, as opposed to a data problem or PEBKAC, it is perfectly moral to assert or throw when the bad case happens. Throwing is probably a bit drastic in this case, so that leaves asserting.

Misuse of any library is bound to cause problems. Libraries are not responsible for anything bad that may happen because of misuse, but it is still nice for libs to have thought ahead a bit to warn the developer that they did something bad. You should design you API with the assumption that it will never be misused, and then assert that assumption wherever you think that it could be misused. It prevents a clunky unnatural API like Destroy(), but still provides the developer enough support so that they can fix their misuses. If you can think of a way to make it fail compile-time, that's even better.

You can do the same thing that the wrapper would have accomplished by overloading operator delete, then call ::delete when you want to invoke the real delete with destructor.

Part of our problem discussing this is that there isn't any code around so my understanding and what is really there may be completely different, and anything I say is based on a bad model.

Here is the code: hparser-0.5a

I don't necessarily intend attempted deletion of an owned node to be an error; the Destruct() function provides a return so the developer can create logic such as "delete if possible, otherwise do something else (like figure out where things went wrong)" or "delete when finished with a branch unless it's owned, otherwise just leave it alone".

Here is a functional example. You don't need to know the lib to understand it really; it just points out how I intend Destruct() to be used.Realistically you wouldn't store anything but the tree base (if that) on the stack. I've just done the above to illustrate a point.
ta0kira

Really, it's just a matter of semantics: what "feels good and natural and expressive" to you? Or the intended users of your application?

If you want to say, "mamma, kill your sons and grandsons," then the order is being addressed to "mamma" -- to the tree. So, in order to "kill Billy," you tell mamma where to point the gun. Anytime that Billy feels a bullet entering his bit-brain, he already knows who shot him and he doesn't have to care who else might be getting the big-send-off at the same time.

Another, equally valid, way to do it is: "Hey Billy, die." Now, Billy knows that he has to tell "mamma" -- the tree -- that he's going away for a lon-n-ng time and won't be coming back. He could also handle this request by telling his mamma, "kill me and all my children." It would work equally well either way.

It will probably be important to distinguish between the external command interface -- the one that someone outside issues to give a particular node a case of rigor mortis -- and the internal notification interfaces which only nodes, trees, and their friends can use. It's one thing to be the recipient of the external command to die, and another thing to be informed that, as a result of your parent's suicide, it's time for you to die as well. Your response is the same, but the reason is not.

Separate from either one of these two interfaces, there's "the destructor," which to my way of thinking should be used only to clean-up this object data-structure; not anything that might be related to it. In other words, if you want to "kill Billy," you might have to call "billy.commit_suicide()," not ~billy().

Finally, it is especially important in destructor-logic to guard against recursion and any stale pointers.