LinuxQuestions.org
Welcome to the most active Linux Forum on the web.
Go Back   LinuxQuestions.org > Forums > Non-*NIX Forums > Programming
User Name
Password
Programming This forum is for all programming questions.
The question does not have to be directly related to Linux and any language is fair game.

Notices


Reply
  Search this Thread
Old 08-28-2006, 11:33 AM   #1
ta0kira
Senior Member
 
Registered: Sep 2004
Distribution: FreeBSD 9.1, Kubuntu 12.10
Posts: 3,078

Rep: Reputation: Disabled
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
 
Old 08-28-2006, 12:20 PM   #2
aluser
Member
 
Registered: Mar 2004
Location: Massachusetts
Distribution: Debian
Posts: 557

Rep: Reputation: 43
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.
Code:
parent->removeChild(child); // deletes child
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.
 
Old 08-28-2006, 01:10 PM   #3
ta0kira
Senior Member
 
Registered: Sep 2004
Distribution: FreeBSD 9.1, Kubuntu 12.10
Posts: 3,078

Original Poster
Rep: Reputation: Disabled
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
 
Old 08-28-2006, 01:32 PM   #4
paulsm4
LQ Guru
 
Registered: Mar 2004
Distribution: SusE 8.2
Posts: 5,863
Blog Entries: 1

Rep: Reputation: Disabled
"delete this" is a common idiom (especially in Microsoft-land) - and it's perfectly acceptable C++.
 
Old 08-28-2006, 01:54 PM   #5
aluser
Member
 
Registered: Mar 2004
Location: Massachusetts
Distribution: Debian
Posts: 557

Rep: Reputation: 43
yeah I wouldn't sweat it too much. Sounds like things will work fine the way you have them.
 
Old 08-28-2006, 02:33 PM   #6
tuxdev
Senior Member
 
Registered: Jul 2005
Distribution: Slackware
Posts: 2,012

Rep: Reputation: 115Reputation: 115
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.
 
Old 08-28-2006, 03:19 PM   #7
ta0kira
Senior Member
 
Registered: Sep 2004
Distribution: FreeBSD 9.1, Kubuntu 12.10
Posts: 3,078

Original Poster
Rep: Reputation: Disabled
Quote:
Originally Posted by tuxdev
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.
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
 
Old 08-28-2006, 03:36 PM   #8
tuxdev
Senior Member
 
Registered: Jul 2005
Distribution: Slackware
Posts: 2,012

Rep: Reputation: 115Reputation: 115
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?
 
Old 08-28-2006, 05:34 PM   #9
mhcox
Member
 
Registered: Aug 2005
Location: Albuquerque, NM
Distribution: Fedora
Posts: 30

Rep: Reputation: 15
Quote:
Originally Posted by ta0kira
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
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.
Code:
class node
{
public:
    typedef boost::shared_ptr<node> node_ptr;
 
    // with appropriate accessor/modifier methods (add_child(), remove_child(), set_parent(), etc.) below.
private:
    node_ptr parent;
    std::list<node_ptr> children;
};
The default node destructor and the boost::shared_ptr will handle most (all?) of the memory management you need.
 
Old 08-28-2006, 07:59 PM   #10
ta0kira
Senior Member
 
Registered: Sep 2004
Distribution: FreeBSD 9.1, Kubuntu 12.10
Posts: 3,078

Original Poster
Rep: Reputation: Disabled
Quote:
Originally Posted by tuxdev
How can it fail exactly? Destructors simply don't fail, and if they possibly could, it is definitely exceptional.
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.
Quote:
Originally Posted by tuxdev
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?
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.
Quote:
Originally Posted by tuxdev
Reiterating a previous query, could you implement this library using an array, map or set, ignoring performance issues? What is this library for, anyway?
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.
Quote:
Originally Posted by mhcox
By making the destructor protected, it can't be a stack variable. That's the point of making it protected.
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
 
Old 08-28-2006, 08:12 PM   #11
tuxdev
Senior Member
 
Registered: Jul 2005
Distribution: Slackware
Posts: 2,012

Rep: Reputation: 115Reputation: 115
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?).
 
Old 08-29-2006, 07:12 AM   #12
ta0kira
Senior Member
 
Registered: Sep 2004
Distribution: FreeBSD 9.1, Kubuntu 12.10
Posts: 3,078

Original Poster
Rep: Reputation: Disabled
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
 
Old 08-29-2006, 10:58 AM   #13
tuxdev
Senior Member
 
Registered: Jul 2005
Distribution: Slackware
Posts: 2,012

Rep: Reputation: 115Reputation: 115
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.
 
Old 08-29-2006, 05:55 PM   #14
ta0kira
Senior Member
 
Registered: Sep 2004
Distribution: FreeBSD 9.1, Kubuntu 12.10
Posts: 3,078

Original Poster
Rep: Reputation: Disabled
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.
Code:
#include <iostream>
#include <assert.h>
#include <hparser/linked-section.hpp>


struct MyNode : public LinkedSection //'LinkedSection' implements linking/memory management
{
	~MyNode()
	{ std::cout << "[DELETED!]\n"; }

	//All of these are overrides of pure virtuals

	StorageSection *Copy() const
	{ return new MyNode(*this); }

	OutputSender* SendData(DataOutput*) const
	{ return NULL; }

	InputReceiver* ReceiveData(DataInput*)
	{ return NULL; }
};


int main()
{
	StorageSection *Node  = NULL; //'StorageSection' = abstract base class of 'LinkedSection'
	StorageSection *Child = NULL;
	Node = new MyNode;

	Node->AddChild(new MyNode);

	Node->AddNext(new MyNode); //A second node owned by 'Node' to illustrate a point


	std::cout << "First delete attempt...\n";

	//Try to delete the child
	if ( !StorageSection::Destruct( Node->Child() ) )
	std::cout << "The added node was not owned so we deleted it.\n";

	else
	{
	std::cout << "The added node was owned so we try to figure it out.\n";

        //Here is the big fix; this replaces the child with NULL which removes it
	Child = Node->SetChild(NULL);

	std::cout << "Second delete attempt...\n";

	//Try to delete the child after the fix was made
	if ( !(Child = StorageSection::Destruct(Child)) )
	std::cout << "The added node was not owned so we deleted it.\n";

	else
	 {
	std::cout << "The added node was STILL owned, so we messed up.\n";
	assert(0);
	 }
	}


	//Using an assignment sets it to NULL if deleted, or back to original if not
	Node = StorageSection::Destruct(Node);

	return 0;
}
Code:
> g++ main.cpp -lhparser -o testme
> ./testme
First delete attempt...
The added node was owned so we try to figure it out.
Second delete attempt...
[DELETED!]
The added node was not owned so we deleted it.
[DELETED!]
[DELETED!]
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
 
Old 08-30-2006, 05:42 PM   #15
sundialsvcs
LQ Guru
 
Registered: Feb 2004
Location: SE Tennessee, USA
Distribution: Gentoo, LFS
Posts: 9,078
Blog Entries: 4

Rep: Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187Reputation: 3187
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.
 
  


Reply


Thread Tools Search this Thread
Search this Thread:

Advanced Search

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off



Similar Threads
Thread Thread Starter Forum Replies Last Post
Bad file problem--can't delete (or anything else) David the H. Linux - General 2 05-12-2006 06:28 AM
remove bad sector form fat? nitin403 Mandriva 1 05-27-2005 03:20 AM
how do I copy a whoel folder form one directory to another form the command line? zwyrbla Linux - Newbie 8 08-24-2004 06:40 PM
delete content form a file bru Programming 1 04-30-2004 07:41 AM
Event driven object-to-object: C++ template class mecanism ( NOT STL or STDC++) bretzeltux Programming 2 12-23-2003 02:45 PM

LinuxQuestions.org > Forums > Non-*NIX Forums > Programming

All times are GMT -5. The time now is 08:02 PM.

Main Menu
Advertisement
My LQ
Write for LQ
LinuxQuestions.org is looking for people interested in writing Editorials, Articles, Reviews, and more. If you'd like to contribute content, let us know.
Main Menu
Syndicate
RSS1  Latest Threads
RSS1  LQ News
Twitter: @linuxquestions
Open Source Consulting | Domain Registration