Is equivalent destruction undefined behavior?

Viewed 128

Will running a 'equivalent' procedure to an objects destructor result in undefined behavior under the standard?

Example:

Assume we wish to represent a directed acyclic tree, with homogeneous nodes, with root ownership. A problem with deep-structures is recursive operations. Almost all operations can be implemented, of course, with an allocated stack. However, can the nodes remain homogeneous, and automatically destroyed, without undefined behavior?

Namely by bypassing recursive destruction by doing a logically equivalent stack based destruction operation from the root node. (Ignoring possible exceptions from allocation)

class Node {
    // ...

    // Assuming unique ownership
    Node *left;
    Node *right;

    ~Node() { destroy_deep(); }

    void destroy()
    {
        // Assert: left == nullptr && right == nullptr
        // ...
    }

    void destroy_deep()
    {
        std::vector<Node *> nodes{ this };



        while (nodes.size()) {
            Node &next = *nodes.back();

            if (!next.left && !next.right) {
                next.destroy();
                
                if (next != this) {
                    // Assume such a deallocate function
                    deallocate(next);
                }
                 
                nodes.pop_back();
            } else {
                if (next.left) {
                    nodes.push_back(next.right);
                }

                if (next.right) {
                    nodes.push_back(next.left);
                }
            }
        }
    }
};
2 Answers

However, can the nodes remain homogeneous, and automatically destroyed, without undefined behavior?

Sure. First of all let's use proper data type for pointers, as you assume unique ownership we should use std::unique_ptr:

class Node {
    using NodePtr = std::unique_ptr<Node>;
// ...

// Assuming unique ownership
   NodePtr left;
   NodePtr right;
};

Ok now this class will properly manage it's data, but it will destroy the tree recursively. Now to avoid recursion we just create our own custom destructor:

Node::~Node()
{
     if( ! (left || right) ) // optimization
         return;
     std::vector<NodePtr> nodes;
     auto populate = [&nodes] ( Node *n ) {
         if( n->left ) nodes.push_back( std::move( n->left ) );
         if( n->right ) nodes.push_back( std::move( n->right ) );
     };
     populate( this );
     while( !nodes.empty() ) {
         auto n = std::move( nodes.back() );
         nodes.pop_back();
         populate( n.get() );
     }
}

Live example

That's it, it will destroy children using std::vector as stack.

The general approach is sound, but your implementation does indeed have undefined behavior. (I hope you did not assume the approach is flawed simply because your implementation crashed.) Take this as a lesson in utilizing the standard library to avoid bugs, as the biggest flaw would have been avoided if you had used std::unique_ptr to model unique ownership instead of relying on a comment.

The problems (undefined behavior)

  1. Infinite loop: Assume the root has children. You start the process by adding the root to the nodes vector. Then you enter the loop and process the root. The root has children, so you push the children onto nodes and go to the next iteration. Suppose we eventually get back to the case where nodes.size() is 1 (otherwise, we already know the loop is infinite). Then the loop will again process the root. The root has children, so you push the children onto nodes and go to the next iteration.
    You can never empty the vector because you never set the child pointers to null (which would have been automatic with unique_ptr). While an infinite loop is bad by itself, in this case it leads to another problem as a side-effect (see the next point).

  2. Multiple de-allocations: Does your assumed deallocate function allow de-allocating the same object multiple times? Probably not. If it mimics the behavior of delete, then it does not. Thanks the the infinite loop, your code would attempt to de-allocate at least one of the leaves an infinite number of times. "Would" except it might crash the second time.
    Note that a similar problem might arise with calling destroy() multiple times. However, a re-entrant destroy is more likely than a re-entrant deallocate.

  3. Null de-reference: When calling back(), the retrieved pointer is immediately de-referenced. This assumes that null pointers are never stored in the vector. However – perhaps this is a typo, but it has a major consequence – null pointers are stored in the vector whenever a node has exactly one child. If a node has a left child (so next.left evaluates to truth), then the right child is pushed on the vector, and vice-versa.

A solution

While the better solution is to use unique_ptr (as in the other answer), your approach is salvageable. After addressing the above problems plus the syntax error present in next != this, your code works. In fact, you don't even need to "Assume such a deallocate function", as delete will work just as well. While delete will call the destructor in addition to de-allocating memory, the destructor is not recursive for leaf nodes. And de-allocation is invoked only for leaf nodes.

If you must use a custom de-allocation function, then just make sure the nodes were created using the corresponding allocation function. Note that using a custom de-allocation function is compatible with using unique_ptr, and in fact using unique_ptr might be a way to ensure that the allocation and de-allocation functions match.

I am assuming that destroy() is re-entrant. If this is not the case, then more adjustments will be needed. Perhaps move the call to destroy() to the destructor (after calling destroy_deep), since there is no longer an issue with too-deep recursion.

Here is your destroy_deep() with the fixes and the assumption that the "deallocate function" is actually delete. Some cleanup could/should still be done; this is supposed to highlight the changes (marked with ///-style comments).

void destroy_deep()
{
    std::vector<Node *> nodes{ this };

    /// STYLE: Using `!empty()`  is clearer than implicitly
    ///        converting `size()` to a boolean.
    while (!nodes.empty()) {
        Node &next = *nodes.back();

        if (!next.left && !next.right) {
            next.destroy();
                
            /// ERROR: Could not compare `Node` to `Node*`.
            ///        So I changed `next` to `&next`.
            if (&next != this) {
                // Assume such a deallocate function
                /// Rather than assume, I'll just delete.
                delete &next;
            }
            nodes.pop_back();
        } else {
            if (next.left) {
                /// ERROR: Original code pushed the *right* child because
                ///        the *left* child is not null???
                nodes.push_back(next.left);
                /// ERROR: Infinite loop without the following:
                next.left = nullptr;
            }

            if (next.right) {
                /// ERROR: Original code pushed the *left* child because
                ///        the *right* child is not null???
                nodes.push_back(next.right);
                /// ERROR: Infinite loop without the following:
                next.right = nullptr;
            }
        }
    }
}

The major fix is setting left and right to null after pushing them onto the vector. You would not have been able to forget this if using unique_ptr since a unique_ptr would have to be moved, not copied, to the vector, and since this move would cause the original pointer to become null.

Related