Jump to content
  • Advertisement
Sign in to follow this  
daniel_i_l

"push_back" calls destructor?

This topic is 3940 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I have a class, let's call it Tree that contains a pointer to the root of a binary tree. (the root has pointers to its kids who have pointers to theirs...). Now, in the destructor of Tree I have a function that recursively deallocates the memory used by the tree. Something like this:
class Node{
private:
   bool IsLeaf;
   Node* child1, child2;
public:
   Node();
   void DeleteChildren();
};

void Node::DeleteChildren()
{
    if(!IsLeaf)
    {
        child1->DeleteChildren();
        child2->DeleteChildren();
        delete child1;
        delete child2;
    }
}
class Tree{
private:
   Node *Root;
public:
   Tree();
   ~Tree();
};

Tree::~Tree
{
   Root->DeleteChildren();
   delete Root;
}


Now, I wanted to make a std vector of trees so I did this: Tree Tree1; vector<Tree> Trees; Trees.push_back(Tree1); But for some reason the program crashed. Sp I ran the debugger and found that in the first line Tree1 was initialized and set up properly, but for some reason, as soon as I called the third line Tree1.~Tree was called and it was all erased. Why does this happen? Thanks.

Share this post


Link to post
Share on other sites
Advertisement
push_back may copy-construct and destruct any number of elements of the vector any number of times. Therefore, it is imperative that copy construction of your objects is safe.

In your case, it isn't, because your ownership policy is sloppy (the sub-trees are owned by the Tree object as well as any of its copies, which makes it impossible to copy your objects without a crash).

Share this post


Link to post
Share on other sites
As std::vector resizes, it will use the copy constructor to create new instances of the objects it contains before destructing the 'old' instances. As you delete the tree in the destructor, the new instance doesn't have good memory to listen to.

I'm guessing the easiest fix would be to drop the manual memory-handling and use smart pointers instead (as, I think, there would then be no need for writing a custom copy-constructor, which is the other option).

...

Also, note that I might be completely wrong (but I think I'm right).

EDIT: Ninja-ed by one minute. On the upside, I was right.

Share this post


Link to post
Share on other sites
Simple answer is because it is supposed to and you are supposed to supply a copy constructor and assignment operator to allow deep copies.
google shallow copy and deep copy.

Share this post


Link to post
Share on other sites
Quote:
Original post by daniel_i_l
Thanks for the replys. How can I improve my ownership policy?
Thanks.


Either write a sane copy-constructor (and assignment operator) or don't manually handle memory when you don't have to — use a smart pointer instead (which will manually delete the memory when it's no longer being used).

In terms of actually improving the state of affairs rather than just fixing the bug, you're better doing both (such that each object owns its own copy of the tree, which is then automatically deleted when that instance is destructed without you having to do anything like provide a manual destructor or have a DeleteNode member which manually uses delete).

Share this post


Link to post
Share on other sites
The Node should call it's own DeleteChildren method in its destructor. And DeleteChildren should delete child1 and 2 only if they're not null.

Share this post


Link to post
Share on other sites
You can also use Reference Counting. That should also fix that problem when implementing a proper copy constructor. Though this way it is easier to write buggy code without smart pointers.

Share this post


Link to post
Share on other sites
Quote:
Original post by Cranky
You can also use Reference Counting. That should also fix that problem when implementing a proper copy constructor.


Whilst a smart pointer saves you having to roll-your-own, which will doubtless end up poorer in some way than those provided in, say, Boost.

EDIT: Note: this post was written before you edited your post.

Share this post


Link to post
Share on other sites
How do I write a copy constructor that copies the whole tree? And is that the only solution? Because I've seen many tutorials that build trees this way and they never mentioned the need for a copy constructor.
Thanks.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!