//don't care about instruction_name and clip_index
struct Instruction{
Instruction * left_child, * right_child;
int instruction_name;
int clip_index;
bool operator<(Instruction & i) {return (instruction_name < i.instruction_name);}
bool operator<(int name) {return (instruction_name < name);}
void Print(int level) {
std::cout << "Level: " << level << " Inst_Name: " << instruction_name << " Clip_Index: " << clip_index << '\n';
++level;
if(left_child) left_child->Print(level);
if(right_child) right_child->Print(level);
}
void CopyTree(Instruction ** root){
(*root) = new Instruction(instruction_name,clip_index);
if(left_child) left_child->CopyTree(&(*root)->left_child);
if(right_child) right_child->CopyTree(&(*root)->right_child);
}
void DeleteTree(){
if(left_child) left_child->DeleteTree();
if(right_child) right_child->DeleteTree();
std::cout << "Node deleted!\n";
delete this; // will this cause any problems?
}
Instruction(int name, int clip) {left_child = right_child = 0; instruction_name = name; clip_index = clip;}
}
delete this?
I am implementing a binary tree and now I want to write a method that deletes the tree. Can I simply call "delete this;" in my method? Here's the code:
Of course you could, but you should make sure that the user of your code can NOT make a static object, and that he will not delete it himself.
I'm not sure how you do the first, but by providing a creation function, you can lower the risk of manual deletion of the object.
The user of your code will not use delete on bla now. If he has a brain.
I'm not sure how you do the first, but by providing a creation function, you can lower the risk of manual deletion of the object.
//Bla* bla = new Bla();Bla* bla = MYLIBCreateBla();bla->Release(); // in this function you do delete this!
The user of your code will not use delete on bla now. If he has a brain.
Quote:delete this; // will this cause any problems?
With what I see there, yes it will. It's legal, but if you read that link Sneftel posted, you are not allocating memory to the class using 'new'. Not only that, the dtor will be called, violating another rule from that link as well. I would suggest that you just use an alternate method, such as Pipo's rather than using 'delete this'.
I don't see any violations of the rules. (I am allocating memory with 'new', you couldn't know that).
Isn't that the purpose of destructors, to get called on 'delete'? I'm quite sure, the dtor is called before the object is deleted.
Since the user will only be me, I won't do this ;-) The tree is only part of another class and if the whole thing will be released (if it will), other users are not likely to use this sub-class. I wouldn't make sense.
Thank you for your replies!
bool State::AddInstruction(int name,int clip){ if(FindInstruction(name) != -1) return false; if(!instructions){ instructions = new Instruction(name,clip); //instructions the root of the tree. it's a pointer to Instruction return true; } Instruction * it = instructions; for(;;){ if((*it) < name){ if(it->left_child) {it = it->left_child;} else {it->left_child = new Instruction(name,clip); return true;} }else{ if(it->right_child) {it = it->right_child;} else {it->right_child = new Instruction(name,clip); return true;} } } }
Quote:
Not only that, the dtor will be called, violating another rule from that link as well.
Isn't that the purpose of destructors, to get called on 'delete'? I'm quite sure, the dtor is called before the object is deleted.
Quote:
The user of your code will not use delete on bla now. If he has a brain.
Since the user will only be me, I won't do this ;-) The tree is only part of another class and if the whole thing will be released (if it will), other users are not likely to use this sub-class. I wouldn't make sense.
Thank you for your replies!
Quote:Original post by Mav
Isn't that the purpose of destructors, to get called on 'delete'? I'm quite sure, the dtor is called before the object is deleted.
...
Since the user will only be me, I won't do this ;-) The tree is only part of another class and if the whole thing will be released (if it will), other users are not likely to use this sub-class. I wouldn't make sense.
Well my point was that what's to stop another member function being called after that? It doesn't matter though, since you are the only user. I know that you know once you call deletetree, you must allocate new memory, but someone else might forget or not do it and wonder why your code crashes [wink]. Cheers!
- Drew
Quote:Original post by MavAs a maintenance programmer, I prefer to initially discourage the usage of "delete this;". In this case you could simply use the destructor.
I am implementing a binary tree and now I want to write a method that deletes the tree. Can I simply call "delete this;" in my method? Here's the code:
*** Source Snippet Removed ***
//Destructor... Instruction::~Instruction(){ delete left_child; delete right_child; std::cout << "Node deleted!\n"; }
But to delete the whole tree you just do: "delete root;". Simple!
Edit: Removed that stupid 'void' typo.
[Edited by - iMalc on March 13, 2005 10:31:55 PM]
Sounds like a good idea. Thank you. Can you explain why exactly you do not want do use "delete this"?
Quote:Original post by iMalc
I am implementing a binary tree and now I want to write a method that deletes the tree. Can I simply call "delete this;" in my method? Here's the code:
*** Source Snippet Removed ***
Destructors can have return types now?
The dtor should not have a return type; that's a typo or brainfart. :)
No. There is an important subtlety here. The purpose of a destructor is to specify code for cleaning up an object's memory before it is disposed.
Objects on the heap are disposed via operator delete (or delete[], if part of an array); this translates into a call to the destructor followed by a call to free(). (Effectively; it's actually a bit different, because 'delete' knows the size of the allocated data whereas 'delete[]' and 'free' do not, so that information can be used to optimize things; also, it may be reimplemented rather than relying on the C runtime.)
Objects on the stack, however, are disposed 'implicitly'; at the end of scope, the destructor is called, and then there is no more to do - the memory is "returned to the system" simply by adjusting the (hardware) stack pointer.
So it's not so much that you don't want to 'delete this' as that you do want to put this logic in a destructor instead of a "DeleteTree" function, because it works much better with the C++ runtime. This way, the child nodes will be cleaned up whenever a parent node is, regardless of how the parent was allocated. Similarly, "CopyTree" should become a copy constructor instead:
The problem is that you are trying to treat new-style C++ objects in old C ways. Member functions that do 'special' things, i.e. that are concerned with the allocation or destruction of instances, should use the appropriate constructs (constructors and destructors).
Another important concept here is that of object ownership. In your case, we are assuming that a parent node 'owns' all the objects under it: i.e., everything except possibly the root will be heap-allocated, and have only one pointer to it (from the parent). That makes it safe to write the destructor in the suggested way. If other nodes in the tree might be stack-allocated, or if it is possible to share nodes between trees, then you have problems (with trying to delete off the stack, or multiple deletion of the same object, respectively).
Unfortunately, given a pointer there is in general no way to tell whether it points to a heap location or a stack location, and given an object there is no built-in way to count the references to it - in C++ anyway. If such management is necessary, it needs to be implemented on top of the language framework. ([google] "C++ smart pointer").
Quote:
Isn't that the purpose of destructors, to get called on 'delete'? I'm quite sure, the dtor is called before the object is deleted.
No. There is an important subtlety here. The purpose of a destructor is to specify code for cleaning up an object's memory before it is disposed.
Objects on the heap are disposed via operator delete (or delete[], if part of an array); this translates into a call to the destructor followed by a call to free(). (Effectively; it's actually a bit different, because 'delete' knows the size of the allocated data whereas 'delete[]' and 'free' do not, so that information can be used to optimize things; also, it may be reimplemented rather than relying on the C runtime.)
Objects on the stack, however, are disposed 'implicitly'; at the end of scope, the destructor is called, and then there is no more to do - the memory is "returned to the system" simply by adjusting the (hardware) stack pointer.
So it's not so much that you don't want to 'delete this' as that you do want to put this logic in a destructor instead of a "DeleteTree" function, because it works much better with the C++ runtime. This way, the child nodes will be cleaned up whenever a parent node is, regardless of how the parent was allocated. Similarly, "CopyTree" should become a copy constructor instead:
// Instead of copying self to some other uninitialized location, we are// copying into self from an existing Instruction, in order to initialize this.// All the data members can be initialized in the initializer list, with a bit// of trickery... we inspect the pointers in 'other', and if they are null we // initialize to null ourselves; otherwise we recursively copy.Instruction::Instruction(const Instruction & other) : instruction_name(other.instruction_name), clip_index(other.clip_index), left_child(other.left_child ? new Instruction(*(other.left_child)) : 0), right_child(other.right_child ? new Instruction(*(other.right_child)) : 0) {}// And that will play quite nicely with the proposed destructor:Instruction::~Instruction() { delete left_child; delete right_child; }// In calling code:{Instruction onStack(SOME_NAME, SOME_CLIP); // normal construction// Add children to onStack here. Those nodes probably will all be on the heap,// but the onStack 'root' is on the stack.Instruction onHeap = new Instruction(onStack); // heap-allocated copy of onStack// The copy constructor is called, and onHeap ends up with copies of all nodes.// Do calculations...delete onHeap; // The destructor is called, and deletes all the children of // onHeap, then the onHeap node itself is freed.} // At end of scope, the same happens with onStack, except that onStack is not// "freed" because it was never a heap allocation. If we'd tried to DeleteTree()// it, then we'd be screwed because of trying to free (in the C sense) a heap// allocation. Besides, we shouldn't have to make that call manually for a// stack allocation...
The problem is that you are trying to treat new-style C++ objects in old C ways. Member functions that do 'special' things, i.e. that are concerned with the allocation or destruction of instances, should use the appropriate constructs (constructors and destructors).
Another important concept here is that of object ownership. In your case, we are assuming that a parent node 'owns' all the objects under it: i.e., everything except possibly the root will be heap-allocated, and have only one pointer to it (from the parent). That makes it safe to write the destructor in the suggested way. If other nodes in the tree might be stack-allocated, or if it is possible to share nodes between trees, then you have problems (with trying to delete off the stack, or multiple deletion of the same object, respectively).
Unfortunately, given a pointer there is in general no way to tell whether it points to a heap location or a stack location, and given an object there is no built-in way to count the references to it - in C++ anyway. If such management is necessary, it needs to be implemented on top of the language framework. ([google] "C++ smart pointer").
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement