Better destructors and partially vibed sorted series remove functions

src/lib/DataStructures/Series.vala

@@ -178,6 +178,8 @@ namespace Invercargill.DataStructures {
                 while(node != null) {
                     SeriesItem<T>* removed = node;
                     node = node->next;
+                    // Delete won't unref the value so explicitly set to null before deleting
+                    removed->value = null;
                     delete removed;
                 }
                 root = null;

src/lib/DataStructures/SortedSeries.vala

@@ -114,7 +114,6 @@ namespace Invercargill.DataStructures {
             }
             else {
                 if(cmp == 0) {
-                    print("Does this actually happen?\n");
                     assert_not_reached ();
                 }
                 else if(cmp < 0) {
@@ -175,16 +174,304 @@ namespace Invercargill.DataStructures {
             }
             root->is_red = false;
         }
+
+        ~SortedSeries() {
+            clear();
+            delete nil;
+        }
+
+        // --- removal methods and helpers ---
+
         public void remove_first_where (PredicateDelegate<T> predicate) {
-            assert_not_reached ();
+            if (root == nil) return;
+
+            // iterative inorder using Vector as stack
+            Vector<SeriesNode<T>*> stack = new Vector<SeriesNode<T>*>();
+            SeriesNode<T>* node = root;
+
+            while (node != null || stack.length != 0) {
+                while (node != null && node != nil) {
+                    stack.add (node);
+                    node = node->left_child;
+                }
+                if (stack.length == 0) break;
+                node = stack[stack.length - 1];
+                stack.remove_at (stack.length - 1);
+
+                // scan values linked list
+                SeriesNodeItem<T>* prev = null;
+                SeriesNodeItem<T>* vi = node->first_value;
+                while (vi != null) {
+                    SeriesNodeItem<T>* next = vi->next;
+                    if (predicate (vi->item)) {
+                        // remove vi from list and delete it
+                        if (prev == null) {
+                            node->first_value = next;
+                            if (node->first_value == null)
+                                node->last_value = null;
+                        } else {
+                            prev->next = next;
+                            if (prev->next == null)
+                                node->last_value = prev;
+                        }
+                        // Delete won't unref the value so explicitly set to null before deleting
+                        vi->item = null;
+                        delete vi;
+                        n_items--;
+                        // if node has no remaining values, remove node from tree (and delete it)
+                        if (node->first_value == null) {
+                            rb_delete_node (node);
+                        }
+                        return;
+                    }
+                    prev = vi;
+                    vi = next;
+                }
+
+                node = node->right_child;
+            }
         }
+
         public void remove_all_where (PredicateDelegate<T> predicate) {
-            assert_not_reached ();
+            if (root == nil) return;
+
+            // collect nodes via inorder traversal first (so tree mutations won't confuse traversal)
+            Vector<SeriesNode<T>*> nodes = new Vector<SeriesNode<T>*>();
+            Vector<SeriesNode<T>*> stack = new Vector<SeriesNode<T>*>();
+            SeriesNode<T>* node = root;
+            while (node != null || stack.length != 0) {
+                while (node != null && node != nil) {
+                    stack.add (node);
+                    node = node->left_child;
+                }
+                if (stack.length == 0) break;
+                node = stack[stack.length - 1];
+                stack.remove_at (stack.length - 1);
+                nodes.add (node);
+                node = node->right_child;
+            }
+
+            // process each collected node (some nodes may already have been removed; skip those)
+            for (uint i = 0; i < nodes.length; i++) {
+                var n = nodes[i];
+                // skip if node is the sentinel or already orphaned (not in tree)
+                if (n == nil) continue;
+                // quick check: if n is not reachable from any parent and isn't root, it may have been removed
+                if (n->parent == null && n != root) continue;
+
+                SeriesNodeItem<T>* prev = null;
+                SeriesNodeItem<T>* vi = n->first_value;
+                while (vi != null) {
+                    SeriesNodeItem<T>* next = vi->next;
+                    if (predicate (vi->item)) {
+                        // remove vi and delete it
+                        if (prev == null) {
+                            n->first_value = next;
+                            if (n->first_value == null)
+                                n->last_value = null;
+                        } else {
+                            prev->next = next;
+                            if (prev->next == null)
+                                n->last_value = prev;
+                        }
+                        // Delete won't unref the value so explicitly set to null before deleting
+                        vi->item = null;
+                        delete vi;
+                        n_items--;
+                        // continue scanning with same prev (since prev didn't change)
+                        vi = next;
+                        continue;
+                    }
+                    prev = vi;
+                    vi = next;
+                }
+                // if node now empty, delete node from tree
+                if (n->first_value == null) {
+                    rb_delete_node (n);
+                }
+            }
         }
+
         public void clear () {
-            assert_not_reached ();
+            if (root == nil) {
+                n_items = 0;
+                return;
+            }
+
+            // postorder traversal to delete all nodes and their items (retain nil sentinel)
+            Vector<SeriesNode<T>*> stack = new Vector<SeriesNode<T>*>();
+            SeriesNode<T>* node = root;
+            SeriesNode<T>* last = null;
+
+            while ((node != null && node != nil) || stack.length != 0) {
+                if (node != null && node != nil) {
+                    stack.add (node);
+                    node = node->left_child;
+                } else {
+                    var peek = stack[stack.length - 1];
+                    if (peek->right_child != nil && last != peek->right_child) {
+                        node = peek->right_child;
+                    } else {
+                        // delete all items in peek
+                        SeriesNodeItem<T>* vi = peek->first_value;
+                        while (vi != null) {
+                            SeriesNodeItem<T>* next = vi->next;
+                            // Delete won't unref the value so explicitly set to null before deleting
+                            vi->item = null;
+                            delete vi;
+                            vi = next;
+                        }
+                        // sever links and delete node (but don't delete sentinel nil)
+                        peek->first_value = null;
+                        peek->last_value = null;
+                        peek->left_child = nil;
+                        peek->right_child = nil;
+                        peek->parent = null;
+                        last = peek;
+                        stack.remove_at (stack.length - 1);
+                        delete peek;
+                    }
+                }
+            }
+
+            // reset structure
+            root = nil;
+            n_items = 0;
+        }
+
+        // --- RB helper functions (use nil sentinel; delete removed node at the end) ---
+
+        private void transplant (SeriesNode<T>* u, SeriesNode<T>* v) {
+            if (u->parent == null) {
+                root = v;
+            } else if (u == u->parent->left_child) {
+                u->parent->left_child = v;
+            } else {
+                u->parent->right_child = v;
+            }
+            // set v->parent even if v == nil (that's OK)
+            if (v != null)
+                v->parent = u->parent;
         }
 
+        private SeriesNode<T>* tree_minimum (SeriesNode<T>* x) {
+            while (x->left_child != nil) {
+                x = x->left_child;
+            }
+            return x;
+        }
+
+        // delete a node z from the tree (assumes z->first_value == null). deletes the node object at the end.
+        // uses standard RB-delete algorithm; x and relatives may be nil sentinel.
+        private void rb_delete_node (SeriesNode<T>* z) {
+            if (z == nil) return;
+            // remember original z to delete at end
+            SeriesNode<T>* z_original = z;
+
+            SeriesNode<T>* y = z;
+            bool y_original_red = y->is_red;
+            SeriesNode<T>* x = nil; // x will be the node that moves into y's original position (or nil)
+
+            if (z->left_child == nil) {
+                x = z->right_child;
+                transplant (z, z->right_child);
+            } else if (z->right_child == nil) {
+                x = z->left_child;
+                transplant (z, z->left_child);
+            } else {
+                y = tree_minimum (z->right_child);
+                y_original_red = y->is_red;
+                x = y->right_child;
+                if (y->parent == z) {
+                    // x->parent should be y (x may be nil)
+                    if (x != null)
+                        x->parent = y;
+                } else {
+                    transplant (y, y->right_child);
+                    y->right_child = z->right_child;
+                    if (y->right_child != null)
+                        y->right_child->parent = y;
+                }
+                transplant (z, y);
+                y->left_child = z->left_child;
+                if (y->left_child != null)
+                    y->left_child->parent = y;
+                y->is_red = z->is_red;
+            }
+
+            // If the removed/moved node was black, fixup
+            if (!y_original_red) {
+                rb_delete_fixup (x);
+            }
+
+            // ensure root is black
+            if (root != nil)
+                root->is_red = false;
+
+            // finally delete the original node (but never delete nil)
+            if (z_original != nil)
+                delete z_original;
+        }
+
+        // fixup when x replaced a black node. x may be nil sentinel.
+        private void rb_delete_fixup (SeriesNode<T>* x) {
+            while (x != root && !x->is_red) {
+                if (x == x->parent->left_child) {
+                    var w = x->parent->right_child;
+                    if (w->is_red) {
+                        w->is_red = false;
+                        x->parent->is_red = true;
+                        rotate_left (x->parent);
+                        w = x->parent->right_child;
+                    }
+                    if ((!w->left_child->is_red) && (!w->right_child->is_red)) {
+                        w->is_red = true;
+                        x = x->parent;
+                    } else {
+                        if (!w->right_child->is_red) {
+                            if (w->left_child != nil)
+                                w->left_child->is_red = false;
+                            w->is_red = true;
+                            rotate_right (w);
+                            w = x->parent->right_child;
+                        }
+                        w->is_red = x->parent->is_red;
+                        x->parent->is_red = false;
+                        if (w->right_child != nil)
+                            w->right_child->is_red = false;
+                        rotate_left (x->parent);
+                        x = root;
+                    }
+                } else {
+                    var w = x->parent->left_child;
+                    if (w->is_red) {
+                        w->is_red = false;
+                        x->parent->is_red = true;
+                        rotate_right (x->parent);
+                        w = x->parent->left_child;
+                    }
+                    if ((!w->right_child->is_red) && (!w->left_child->is_red)) {
+                        w->is_red = true;
+                        x = x->parent;
+                    } else {
+                        if (!w->left_child->is_red) {
+                            if (w->right_child != nil)
+                                w->right_child->is_red = false;
+                            w->is_red = true;
+                            rotate_left (w);
+                            w = x->parent->left_child;
+                        }
+                        w->is_red = x->parent->is_red;
+                        x->parent->is_red = false;
+                        if (w->left_child != nil)
+                            w->left_child->is_red = false;
+                        rotate_right (x->parent);
+                        x = root;
+                    }
+                }
+            }
+            x->is_red = false;
+        }
 
         private void rotate_left(SeriesNode<T>* x) {
             SeriesNode<T>* y = x->right_child;

src/tests/Integration/OrderBy.vala

@@ -11,6 +11,15 @@ void order_by_tests() {
         assert_true(expected_ids.matches(result.select<int>(i => i.id), (a, b) => a == b));
     });
 
+    Test.add_func("/invercargill/operator/order_by/delete", () => {
+        var items = ClassToOrder.data();
+        //  var expected_ids = Wrap.array(new int[] { 1, 2, 3, 4, 5, 6, 7, 8 });
+
+        //  var result = items.order_by<int>(i => i.id);
+        
+        //  assert_true(expected_ids.matches(result.select<int>(i => i.id), (a, b) => a == b));
+    });
+
     Test.add_func("/invercargill/operator/order_by/descending_int", () => {
         var items = ClassToOrder.data();
         var expected_ids = Wrap.array(new int[] { 8, 7, 6, 5, 4, 3, 2, 1 });
@@ -60,12 +69,17 @@ class ClassToOrder {
     public int id { get; set; }
 
     public ClassToOrder(string name, int birth_year, int priority, int id) {
+        print(@"Hi $(name)\n");
         this.name = name;
         this.birth_year = birth_year;
         this.priority = priority;
         this.id = id;
     }
 
+    ~ClassToOrder() {
+        print(@"Bye $(name)\n");
+    }
+
     public static Enumerable<ClassToOrder> data() {
         var series = new DataStructures.Series<ClassToOrder>();
         series.add(new ClassToOrder("Bob", 1997, 4, 4));
@@ -79,7 +93,4 @@ class ClassToOrder {
         return series;
     }
 
-    ~ClassToOrder() {
-        print(@"Bye $(name)\n");
-    }
 }

src/tests/Integration/SortedSeries.vala

@@ -38,7 +38,40 @@ void sorted_series_tests() {
         series.add_all(items);
         assert_cmpint(series.count(), CompareOperator.EQ, 200);
 
-        series.matches(items.interleave(items), (a, b) => a == b);
+        assert_true(series.matches(items.interleave(items), (a, b) => a == b));
+    });
+
+    Test.add_func("/invercargill/structure/sorted_series/remove_first_where", () => {
+
+        var series = new SortedSeries<int>();
+        var items = range(0, 100);
+        series.add_all(items);
+        series.add_all(items);
+        items.iterate(i => series.remove_first_where(si => si == i));
+        assert_cmpint(series.count(), CompareOperator.EQ, 100);
+        assert_true(series.matches(items, (a, b) => a == b));
+    });
+
+    Test.add_func("/invercargill/structure/sorted_series/remove_all_where", () => {
+
+        var series = new SortedSeries<int>();
+        var items = range(0, 100);
+        series.add_all(items);
+        series.add_all(items);
+        series.remove_all_where(si => si < 50);
+        assert_cmpint(series.count(), CompareOperator.EQ, 100);
+        assert_true(range(50, 100).interleave(range(50, 100)).matches(series, (a, b) => a == b));
+    });
+
+    Test.add_func("/invercargill/structure/sorted_series/clear", () => {
+
+        var series = new SortedSeries<int>();
+        var items = range(0, 100);
+        series.add_all(items);
+        series.add_all(items);
+        series.clear();
+        assert_cmpint(series.count(), CompareOperator.EQ, 0);
+        assert_cmpint(series.last_or_default(), CompareOperator.EQ, 0);
     });
 
     Test.add_func("/invercargill/structure/sorted_series/to_array", () => {

src/tests/TestRunner.vala

@@ -32,9 +32,8 @@ public static int main(string[] args) {
     buffer_speed_test();
     set_speed_test();
     dictionary_speed_test();
-    //  sorted_series_speed_test(); // Disabled until destructor finished!
+    sorted_series_speed_test();
     fifo_speed_test();
 
-
     return result;
 }