views:

133

answers:

4

I have recently managed to get a stack overflow when destroying a tree by deleting its root 'Node', while the Node destructor is similar to this:

Node::~Node(){
    for(int i=0;i<m_childCount;i++)
        delete m_child[i];
}

A solution that come up into my mind was to use own stack. So deleting the tree this way:

std::stack< Node* > toDelete;

if(m_root)
    toDelete.push(m_root);

while(toDelete.size()){
    Node *node = toDelete.top();
    toDelete.pop();

    for(int i=0;i<node->GetChildCount();i++)
        toDelete.push(node->Child(i));

    delete node;
}

But in there the std::stack::push() may throw an exception. Is it possible to write an exception free tree destruction? How?

EDIT:

If anybody is interested here is an exception free non-recursive code inspired by the algorithm pointed out by jpalecek:

Node *current = m_root;

while(current){
  if(current->IsLeaf()){
    delete current;
    return;
  }

  Node *leftMostBranch = current;// used to attach right subtrees

  // delete all right childs
  for(size_t i=1; i<current->GetChildCount(); i++){
    while(!leftMostBranch->Child(0)->IsLeaf())
      leftMostBranch = leftMostBranch->Child(0);

    delete leftMostBranch->Child(0);
    leftMostBranch->Child(0) = current->Child(i);
  }

  // delete this node and advance to the left child
  Node *tmp = current;
  current = current->Child(0);
  delete tmp;
}

note: Node::IsLeaf() is equivalent to Node::GetChildCount()!=0.

A: 

Why is the original code throwing an exception? I'm guessing you are doing something like using the same node object in multiple places in the tree. Stack overflows are rarely caused by normal expected situations. Stack overflows are not a problem, they are the symptom of a problem.

Rewriting the code differently won't fix that; you should just investigate & fix the error.

tenfour
That is not true! In his case, a stack overflow will be caused by the recursive call to `Node::~Node()`. No matter how correct the tree is, if it is sufficiently large, it *will* cause a stack overflow.
Job
yes absolutely. but the tree would have to be (most likely) abnormally large for that to happen. I'm not saying that legit SO's never happen, i'm saying when you encounter one in your code, your mentality should usually be to first assume it's the symptom of a problem rather than expected behavior. Of course it depends on the situation, and ultimately the best thing is to understand the bug, whether it's a legit SO or not :)
tenfour
Yes, the tree is extremely large.
Juraj Blaho
@Juraj subjective terms like "extremely large" are unacceptable coming from you, since YOU are the one posting the question. This is because YOU know the size of the tree, and the rest of us don't. Rather than just telling us "it's large," why don't you tell us the size of the tree, and we can move on when necessary? It's like fixing a lamp... make sure it's plugged in and getting power before you tear it apart to find out why it doesn't turn on.
San Jacinto
@San Jacinto: It was just to tell that the problem really is a stack overflow caused by recursive call to Node::~Node(). The code of that destructor is not that simple as I wrote in the question. So the stack has filled up much faster. Reason why I posted this simplified/concept of destructor was that I wanted a general answer to the tree destruction problem.
Juraj Blaho
@Juraj, for the future, it's best to clarify assumptions in your question. the people answering need as much context as possible to help you.
tenfour
@San Jacinto- Right on. Just remember to unplug the lamp before taking it apart. :-)
Amardeep
@tenfour: You are assuming a normal desktop machine. C++ is used in many situations where memory is limited. The easy example is simple hardware. But even on desktop machines there are certain situations were you want to limit rampent memory usage (like when writting code in the kernal. In this situation you would prefer to use a loop to traverse the tree rather than doing a depth or breadth first traversal which both require using some form of dynamic memory).
Martin York
i didn't assume anything; without context I'm giving practical advice.
tenfour
A: 

This is what all garbage collectors struggle with. However, the best thing you can do (IMHO) is to pray for enough memory for the stack, and your prayers will be heard 99.99999% of the time. Should it not happen, just abort().

BTW if you are interested, there is a solution to traverse long (and deep) trees without allocating much memory.

jpalecek
"just `abort()`" isn't an answer if the software is to be used for anything important.
Mike Seymour
@Mike Seymour: If the software is to be used for anything important, you have to **ensure** it will never run out of memory, because that already means the mission is failed (the majority of today's code is not meant for "anything important"). And still, if you need it to work under scarce memory condition, the second part of my answer applies.
jpalecek
The algorithm has inspired me. I will try to adapt it. The tree rebuilding is a key.
Juraj Blaho
@jpalecek: This is NOT what garbage disposal struggles with. In fact this is one of the very easy things the GC copes with very well. The hard problem for GC is cyclic dependencies.
Martin York
A: 

Is it possible to write an exception free tree destruction? How?

Perhaps this (untested code):

void destroy(Node* parent)
{
  while (parent)
  {
    //search down to find a leaf node, which has no children
    Node* leaf = parent;

    while (leaf->children.count != 0)
      leaf = leaf->chilren[0];

    //remember the leaf's parent
    parent = leaf->parent;

    //delete the leaf
    if (parent)
    {
      parent->children.remove(leaf);
    }
    delete leaf; 

  } //while (parent)
}
ChrisW
It seems it may work. The only problem is you need an extra pointer to a parent in every node. But that may not be a big problem.
Juraj Blaho
@Juraj Alternatively you can implement it using a local variable-length collection in which you remember the list of direct ancestors from the current node to the original root.
ChrisW
Yes, but that would not be exception free, because when you enlarge the collection it may throw. I have edited the original post and now it contains the algorithm I have crafted based on the answers here.
Juraj Blaho
+3  A: 

I just had this as an interview question.

And I must admit this is one of the hardest things I had to solve on the fly.
Personally I don't think it's a good question as you may know the trick (if you have read Knuth) in which case it becomes trivial to solve but you can still fool the interviewer into making him/her think you have solved it on the fly.

This can be done assuming that the node stores child pointers in a static structure. If the node stores child pointers in a dynamic structure then it will not work, as you need to re-shape the tree on the fly (it may work but there is no guarantee).

Surprisingly the solution is O(n)
(Technically every node is visited exactly twice with no re-scanning of the tree).
This solution uses a loop (so no memory usage for stack) and does not dynamically allocate memeroy to hold nodes that need to be deleted. So it is surprisingly effecient.

class Node
{
    // Value we do not care about.
    int   childCount;
    Node* children[MAX_CHILDREN];
 };

freeTree(Node* root)
{
    if (root == NULL)
    {    return;
    }
    Node* bottomLeft  = findBottomLeft(root);

    while(root != NULL)
    {
        // We want to use a loop not recursion.
        // Thus we need to make the tree into a list.
        // So as we hit a node move all children to the bottom left.
        for(int loop = 1;loop < root->childCount; ++loop)
        {
            bottomLeft->children[0] = root->children[loop];
            bottomLeft->childCount  = std::max(1, bottomLeft->childCount);
            bottomLeft = findBottomLeft(bottomLeft);
        }

        // Now we have a root with a single child
        // Now we can delete the node and move to the next node.
        Node* bad = root;
        root      = root->children[0];
        delete bad;     // Note the delete should no longer destroy the children.
    }
}

Node* findBottomLeft(Node* node)
{
    while((node->childCount > 0) && node->children[0] != NULL))
    {    node = node->children[0];
    }
    return node;
}

The above method will work as long as their is always a children[0] node (even if it is NULL). As long as you do not have to dynamically allocate space to hold children[0]. Alternatively just add one more pointer to the node object to hold the delete list and use this to turn the tree into a list.

Martin York