I am using the Red-Black tree implementation written by Julian Bucknall in his well-known book, The Tomes Of Delphi. Source code can be downloaded here, and I am using the code as-is in Delphi 2010, with modifications to TdBasics.pas
to let it compile in a modern version of Delphi (mostly commenting most of it out - only a few definitions are required by the tree code.)
This is a well-known implementation by a famous author, in an often-recommended book. I feel I should be on solid ground using it. But I am encountering crashes using Delete()
and Promote()
. Stepping back to write unit tests with DUnit, these problems are easily reproducible. Some example code is (snippets from my DUnit tests):
// Tests that require an initialised tree start with one with seven items
const
NumInitialItems : Integer = 7;
...
// Data is an int, not a pointer
function Compare(aData1, aData2: Pointer): Integer;
begin
if NativeInt(aData1) < NativeInt(aData2) then Exit(-1);
if NativeInt(aData1) > NativeInt(aData2) then Exit(1);
Exit(0);
end;
// Add seven items (0..6) to the tree. Node.Data is a pointer field, just cast.
procedure TestTRedBlackTree.SetUp;
var
Loop : Integer;
begin
FRedBlackTree := TtdRedBlackTree.Create(Compare, nil);
for Loop := 0 to NumInitialItems - 1 do begin
FRedBlackTree.Insert(Pointer(Loop));
end;
end;
...
// Delete() crashes for the first item, no matter if it is 0 or 1 or...
procedure TestTRedBlackTree.TestDelete;
var
aItem: Pointer;
Loop : Integer;
begin
for Loop := 1 to NumInitialItems - 1 do begin // In case 0 (nil) causes problems, but 1 fails too
aItem := Pointer(Loop);
Check(FRedBlackTree.Find(aItem) = aItem, 'Item not found before deleting');
FRedBlackTree.Delete(aItem);
Check(FRedBlackTree.Find(aItem) = nil, 'Item found after deleting');
Check(FRedBlackTree.Count = NumInitialItems - Loop, 'Item still in the tree');
end;
end;
I'm not solid enough in the algorithms to know how to fix it without introducing further problems (unbalanced or incorrect tree.) I know, because I've tried :)
The crashing code
The above test fails in Promote()
when deleting an item, on the line marked !!!
:
function TtdRedBlackTree.rbtPromote(aNode : PtdBinTreeNode)
: PtdBinTreeNode;
var
Parent : PtdBinTreeNode;
begin
{make a note of the parent of the node we're promoting}
Parent := aNode^.btParent;
{in both cases there are 6 links to be broken and remade: the node's
link to its child and vice versa, the node's link with its parent
and vice versa and the parent's link with its parent and vice
versa; note that the node's child could be nil}
{promote a left child = right rotation of parent}
if (Parent^.btChild[ctLeft] = aNode) then begin
Parent^.btChild[ctLeft] := aNode^.btChild[ctRight];
if (Parent^.btChild[ctLeft] <> nil) then
Parent^.btChild[ctLeft]^.btParent := Parent;
aNode^.btParent := Parent^.btParent;
if (aNode^.btParent^.btChild[ctLeft] = Parent) then //!!!
aNode^.btParent^.btChild[ctLeft] := aNode
else
aNode^.btParent^.btChild[ctRight] := aNode;
aNode^.btChild[ctRight] := Parent;
Parent^.btParent := aNode;
end
...
Parent.btParent
(becoming aNode.btParent
) is nil
, thus the crash. Examining the tree structure, the node's parent is the root node, which obviously has a nil
parent itself.
Some non-working attempts at fixing it
I tried simply testing for this and only running that if/then/else statement when a grandparent existed. While this seems logical, it's kind of a naive fix; I don't understand the rotations well enough to know if this is valid or if something else should happen instead - and doing so causes another problem, mentioned after the snippet. (Note there is a duplicate of this code below the snippet copied above for a left rotation, and the same bug occurs there too.)
if aNode.btParent <> nil then begin //!!! Grandparent doesn't exist, because parent is root node
if (aNode^.btParent^.btChild[ctLeft] = Parent) then
aNode^.btParent^.btChild[ctLeft] := aNode
else
aNode^.btParent^.btChild[ctRight] := aNode;
aNode^.btChild[ctRight] := Parent;
end;
Parent^.btParent := aNode;
...
Using this code, the test for Delete still fails, but with something more odd: after the call to Delete(), the call to Find() correctly returns nil, indicating the item was removed. However, the last iteration of the loop, removing item 6, causes a crash in TtdBinarySearchTree.bstFindItem
:
Walker := FBinTree.Root;
CmpResult := FCompare(aItem, Walker^.btData);
FBinTree.Root
is nil
, crashing when calling FCompare
.
So - at this point I can tell my modifications are clearly just causing more problems, and something else more fundamental is wrong with the code implementing the algorithm. Unfortunately, even with the book as reference, I can't figure out what is wrong, or rather, what a correct implementation would look like and what's different here.
I originally thought it must have been my code incorrectly using the tree, causing the problems. This is still very possible! The author, the book and thus implicitly the code are well-known in the Delphi world. But the crashes are easily reproducible, writing some very basic unit tests for the class, using the book's source code downloaded from the author's site. Someone else must have also used this code sometime in the past decade, and encountered the same problem (unless the bug is mine and both my code and unit tests are using the tree incorrectly.) I am seeking answers helping with:
- Identifying and fixing any bugs in
Promote
and elsewhere in the class. Note that I have also written unit tests for the base class,TtdBinarySearchTree
, and those all pass. (That doesn't mean it's perfect - I might not have identified failing cases. But it's some help.) - Finding an updated version of the code. Julian hasn't published any errata for the red-black tree implementation.
- If all else fails, finding a different, known good implementation of a red-black tree for Delphi. I am using the tree to solve a problem, not for the exercise of writing a tree. If I have to, I will happily replace the underlying implementation with another (given okay licensing terms etc.) Nevertheless, given the pedigree of the book and code, problems are surprising, and solving them would help more people than just me - it's a widely recommended book in the Delphi community.
Edit: Further notes
Commenter MBo points out Julian's EZDSL library, which contains another implementation of a red-black tree. Unit tests on this version pass. I am currently comparing the two sources to try to see where the algorithms deviate, to find the bug.
One possibility is to simply use the EZDSL red-black tree, not the Tomes of Delphi red-black tree, but there are a few problems with the library that make me not keen to use it: It's written for 32-bit x86 only; some methods are provided in assembly only, not Pascal (though most have two versions); the trees are structured quite differently, such as using cursors to nodes instead of pointers - a perfectly valid approach, but an example of how different the code is to the 'example' code in the ToD book, where navigation is semantically different; the code is, in my opinion, much harder to understand and use: it's quite heavily optimised, variables and methods are as not as clearly named, there are a variety of magic functions, the node structure is actually a union / case record, squishing in details for stacks, queues, dequeues and lists, double-linked-lists, skips lists, trees, binary trees and heaps all in one structure that is almost incomprehensible in the debugger, etc. It's not code I am keen to use in production where I will need to support it, nor is it easy to learn from. The Tomes of Delphi source code is much more readable and much more maintainable... but also incorrect. You see the dilemma :)
I am attempting to compare the code to try to find differences between Julian's in-practice code (EZDSL) and his teaching code (Tomes of Delphi.) But this question is still open and I will still be grateful for answers. I can't be the only person to use the red-black trees from the Tomes of Delphi in the twelve years since it was published :)
Edit: further further notes
I've answered this myself (in spite of offering a bounty. Oops.) I had trouble finding the bugs purely by examining the code and comparing to the ToD description of the algorithm, so instead I reimplemented the flawed methods based on a good page describing the structure that came with a MIT-licensed C implementation; details below. One bonus is that I think the new implementation is actually much clearer to understand.
I haven't managed to figure out what's wrong by examining the Tomes of Delphi source code and comparing to either the algorithm or Julian's other implementation, the heavily-optimised EZDSL library implementation (thus this question!), but I have instead re-implemented
Delete
, and for good measure alsoInsert
, based on the example C code for a red-black tree on the Literate Programming site, one of the clearest examples of a red-black tree I found. (It's actually quite a hard task to find a bug purely by grinding through the code and verifying it implements something correctly, especially when you don't fully understand the algorithm. I can tell you, I have a much better understanding now!) The tree is quite well documented - I think the Tomes of Delphi gives a better overview of the reasons for why the tree works as it does, but this code is a better example of a readable implementation.Notes about this:
FHead
node, the child of which is the tree's root, which you have to be aware of when converting. (Tests often tested if a node's parent was NULL as a way of testing if the node was the root node. I have extracted this and other similar logic to helper methods, or node or tree methods.)TtdBinaryTree
,TtdBinarySearchTree
andTtdRedBlackTree
found inTDBinTre.pas
(source code download on the ToD page.) To use it, edit that file. It's not a new implementation, and isn't complete on its own. Specifically, it keeps the ToD code's structure, such asTtdBinarySearchTree
not being a descendant ofTtdBinaryTree
but owning one as a member (ie wrapping it), using aFHead
node instead of a nil parent to theRoot
, etc.RotateLeft
andRotateRight
), which I find useful - the ToD code talks about rotations but doesn't explicitly pull them into separate named methods.Delete
is similar: it goes into a number of cases. Each case is explained on the page, and as comments in my code. Some of these I named, but some are too complex to put in a method name, so are just "case 4", "case 5" etc, with comments explaining.On to the code!
Node modifications
I added the following helper methods to the node, to make the code more literate when reading. For example, the original code often tested if a node was the left child of its parent by testing (blind conversion to Delphi and unmodified ToD structures)
if Node = Node.Parent.btChild[ctLeft] then...
whereas now you can testif Node.IsLeft then...
etc. The method prototypes in the record definition aren't included to save space, but should be obvious :)I also added extra methods like the existing
IsRed()
, to test if it is black (IMO code scans nicer if it saysif IsBlack(Node)
notif not IsRed(Node)
, and to get the colour, including handling a nil node. Note that these need to be consistent -IsRed
, for example, returns false for a nil node, so a nil node is black. (This also ties in to the properties of a red-black tree, and the consistent number of black nodes on a path to a leaf.)Red-black constraint verification
As mentioned above, these methods verify the structure of the tree and the red-black constraints, and are a direct translation of the same methods in the original C code.
Verify
is declared as inline if not debug in the class definition. If not debug, the method should be empty and will hopefully be completely removed by the compiler.Verify
is called at the beginning and end of theInsert
andDelete
methods, to ensure the tree was correct before and after modification.Rotations and other useful tree methods
Helper methods to check if a node is the root node, to set a node as the root, to replace one node with another, to perform left and right rotations, and to follow a tree down the right-hand nodes to the leaf. Make these protected members of the red-black tree class.
Insertion and deletion
The red-black tree is a wrapper around an internal tree,
FBinTree
. In a too-connected manner this code modifies the tree directly. BothFBinTree
and the wrapper red-black tree keep a countFCount
of the number of nodes, and to make this cleaner I removedTtdBinarySearchTree
(the ancestor of the red-black tree)'sFCount
and redirectedCount
to returnFBinTree.Count
, i.e. ask the actual internal tree that the binary search tree and red-black tree classes use - which is after all the thing that owns the nodes. I've also added notification methodsNodeInserted
andNodeRemoved
to increment and decrement the counts. Code not included (trivial).I also extracted some methods for allocating a node and disposing of a node - not to insert or delete from the tree or do anything about a node's connections or presence; these are to look after creation and destruction of a node itself. Note that node creation needs to set the node's color to red - color changes are looked after after this point. This also ensures that when a node is freed, there is an opportunity to free the data associated with it.
With these extra methods, use the following code for insertion and deletion. Code is commented, but I recommend you read the original page and also the Tomes of Delphi book for an explanation of rotations, and the various cases that the code tests for.
Insertion
Deletion
Final notes
Bucknall writes that his implementation of binary tree uses dummy head node as parent of root node (to avoid special cases). This head is created in constructor:
and used during the first node insertion:
So your situation
"the node's parent is the root node, which obviously has a nil parent itself"
looks very strange unless you have rewritten the key methods