Binary Tree Rotation

I'm working on implementing an AVL Search tree. So far I've finished the coding part and I've started testing it for bugs. I found out that my node rotation methods are bugged and for god's sake I can't understand what's the problem.

The algorithm works as it should on paper but when executed on a machine it well...leaks tree nodes.

This is the method used to rotate a node to the left: http://pastebin.com/mPHj29Af

bool avl_search_tree::avl_tree_node::rotate_left()
{
    if (_right_child != NULL) {
        avl_tree_node *new_root = _right_child;
 
        if (_parent != NULL) {
            if (_parent->_left_child == this) {
                _parent->_left_child = new_root;
            } else {
                _parent->_right_child = new_root;
            }
        }
 
        new_root->_parent = _parent;
        _parent = new_root;
 
        _right_child = new_root->_left_child;
        new_root->_left_child = this;
 
        if (_right_child != NULL) {
            _right_child->_parent = this;
        }
 
        //update heights
        update_height();
        new_root->update_height();
 
        return true;
    }
 
    return false;
}

In my insertion method I commented the AVL balancing part and instead I'm just trying to rotate the newly inserted node to the left. The result for inserting integers in ascending order: my tree only contains the initial root (first node inserted) and all other nodes are leaked.

Any help in identifying the problem is highly appreciated as I'm starting to go mad.

For the record: if I don't use any rotations the tree won't leak nodes and it works as a normal unbalanced binary search tree (for insertion and lookup).

Edit: Due to AJG85's comment I'll add the observations:

I added printf 'checks' to the destructor method of avl_search_tree::avl_tree_node that will print the key value (in my case 32 bit integers) before cleanup and, to the insert method of the avl_search_tree that will print the key just inserted.

Then in the program's entrypoint I allocate an avl_search_tree on the heap and add keys to it in ascending order and then delete it.

With AVL Balancing enabled I get the following output in the terminal:

bool avl_search_tree::insert(const int&) : 1
bool avl_search_tree::insert(const int&) : 2
bool avl_search_tree::insert(const int&) : 3
bool avl_search_tree::insert(const int&) : 4
bool avl_search_tree::insert(const int&) : 5
bool avl_search_tree::insert(const int&) : 6
bool avl_search_tree::insert(const int&) : 7
bool avl_search_tree::insert(const int&) : 8
avl_search_tree::avl_tree_node::~avl_tree_node() : 1

Which means thatall insertions were successful but only the root has been deleted.

With the AVL Balancing commented out it works like a normal binary search tree. The terminal output is:

bool avl_search_tree::insert(const int&) : 1
bool avl_search_tree::insert(const int&) : 2
bool avl_search_tree::insert(const int&) : 3
bool avl_search_tree::insert(const int&) : 4
bool avl_search_tree::insert(const int&) : 5
bool avl_search_tree::insert(const int&) : 6
bool avl_search_tree::insert(const int&) : 7
bool avl_search_tree::insert(const int&) : 8
avl_search_tree::avl_tree_node::~avl_tree_node() : 1
avl_search_tree::avl_tree_node::~avl_tree_node() : 2
avl_search_tree::avl_tree_node::~avl_tree_node() : 3
avl_search_tree::avl_tree_node::~avl_tree_node() : 4
avl_search_tree::avl_tree_node::~avl_tree_node() : 5
avl_search_tree::avl_tree_node::~avl_tree_node() : 6
avl_search_tree::avl_tree_node::~avl_tree_node() : 7
avl_search_tree::avl_tree_node::~avl_tree_node() : 8

Which means that everything is properly cleaned up.

Now...how did I come to the conclusion that the rotation methods are the issues? Under the commented AVL balancing subroutine I added a line that rotates every newly inserted node to the left. The result? The same as if the AVL Balancing subroutine was enabled.

And regarding the update_height() method, It doesn't alter the tree's structure in any way.

I hope this will clarify it.

Edit 2:

To clarify some more things, his is how the avl_tree_node destructor is implemented:

avl_search_tree::avl_tree_node::~avl_tree_node()开发者_开发百科
{
    printf("%s : %d\n", __PRETTY_FUNCTION__, *_key);

    if (_left_child != NULL) {
        delete _left_child;
    }

    if (_right_child != NULL) {
        delete _right_child;
    }

    if (_key != NULL) {
        delete _key;
    }
}

_left_child and _right_child are pointers to avl_tree_node objects allocated on the heap.

Edit 3:

Thanks to AGJ85's 2nd comment I found the issue. In my rotate methods I forgot that I actually have to update the tree's root pointer to the new root whenever the root was shifted.

Basically the tree's root was always pointing to the first inserted node and without updating the pointer when needed, my rotate methods would leak the new tree's root which was actually configured right. :)

Thank you AGJ85!

Thanks to AGJ85's 2nd comment I found the issue. In my rotate methods I forgot that I actually have to update the tree's root pointer to the new root whenever the root was shifted.

Basically the tree's root was always pointing to the first inserted node and without updating the pointer when needed, my rotate methods would leak the new tree's root which was actually configured right. :)

EDIT - Damn - I didn't see that the issue is already solved (answer in question). Still, maybe there's some non-answer tips in this worth salvaging.

I haven't checked thoroughly, but I think you're going wrong at this line...

_right_child = new_root->_left_child;

and that the problem is that you may have already overwritten new_root->_left_child in the line...

_parent->_left_child = new_root;

What I think you should do is, at the start, have a block of local definitions like...

avl_tree_node *orig_parent      = _parent;
avl_tree_node *orig_this        = this;
avl_tree_node *orig_left_child  = _left_child;
avl_tree_node *orig_right_child = _right_child;

Then use the orig_ local variables as the sources for later assignments. This saves a certain amount of worrying about data flow through the various pointers during the rotation. The optimiser should get rid of any redundant work worth worrying about in this, and there isn't much of that anyway.

A couple of extra points...

First, the C++ (and C) standards reserve identifiers with leading underscores, and with double-underscores. It's claimed that you can get surprise interactions with standard and compiler-supplied libraries if you don't respect that - I guess that'd have to be macro-related for member identifiers, though. Trailing underscores are OK - I tend to use them for include guards.

A common convention for member variables is to add a leading m or m_. Even more common, probably, is not having any special prefix or suffix at all.

Secondly, you may (or may not) find it easier to implement AVL trees that don't have parent links stored in the nodes. I haven't implemented AVL trees yet myself, but I did implement red-black trees once. A number of algorithms need to include a recursive search as the first step - you can't just do a standard search that remembers the found node but discards the route down to that node. However, recursive implementation isn't too bad, and there's fewer pointers to juggle.

Finally, a general tip - trying to "dry run" an algorithm like this can easily trip you up unless you strictly work through it step by step, and check all the sources of information that are relevant (have I already modified this?) at every step. It's very easy to get into the habit of skipping some of the details for speed. A useful machine-assisted dry run is to run the code step-by-step in a debugger, and see if the results at every step agree with your paper dry-run.

EDIT - one more note - I won't call this a tip because I'm not sure in this context. I usually implement data structure nodes with simple structs - no data hiding, few if any member functions. Most of the code is kept separate from the data structure, often in a "tool" class. I know this breaks the old "the shape draws itself" OOP principle, but IMO it works better in practice.

I see you have found the bug you were looking for in your code. (As you say, you weren't updating the tree root pointer to the new root when the root changed. It's a common paradigm for list and tree Insert/Delete methods to return a pointer to head of list or root of tree, and if you remember that paradigm won't make the mistake again.)

At a higher level of view, the technique I've used to avoid problems with AVL Tree or Red-Black Tree code is to instead use an AA Tree, which has similar performance to them, using O(n) space and O(log n) time for Insert, Delete, and Search. However, AA trees are significantly simpler to code.