diff --git a/Map.hpp b/Map.hpp index 248e1e3..fc51398 100644 --- a/Map.hpp +++ b/Map.hpp @@ -214,6 +214,10 @@ template class Map { assert(false); } + // erase_child fully deletes the child node, this includes + // having the child's memory be freed so any data the child holds + // which is needed for future calculations must be gathered before + // it is erased void erase_child(Node *n) { this->erase_child(this->which_child(n)); } void erase_child(Direction dir) { // if the child being erased is the min or the max we need to update @@ -754,23 +758,22 @@ private: // the hard case of removing a black leaf node in a red black tree // All other cases are handled in core_erase - void hard_erase(Node *n) { - assert(n->parent); - Node *parent = n->parent; - Direction dir = parent->which_child(n); - parent->erase_child(n); + void hard_erase(Node *target) { + assert(target->parent); - // case 1 should be handled first for all but the first loop - // so it's skipped in the first loop - goto skip; + // parent and direction need to be gathered here due to imminent + // deletion of the target from memory + Node *parent = target->parent; + Direction dir = parent->which_child(target); + + // Note: target is now deallocated and should not be used again + // until we reassign it in the loop + parent->erase_child(target); + + // refer to + // https://en.wikipedia.org/wiki/Red%E2%80%93black_tree#Notes_to_the_delete_diagrams + // for more details on what all of this is ultimately doing while (true) { - parent = n->parent; - if (parent == nullptr) { - // we're at root we're done (case 1) - return; - } - dir = parent->which_child(n); - skip: Color par_color = parent->color; Node *sibling = parent->child(!dir); @@ -782,15 +785,11 @@ private: Color close_color = close ? close->color : Color::Black; Color distant_color = distant ? distant->color : Color::Black; - // Macro is used for simplicity and avoiding repetition + // Macro is used for avoiding repetition #define redcheck(v) if ((v) == Color::Red) - // TODO: continue comment writing here - // it kinda sucks but I think that goto is genuinely the best solution - // here, making methods for cases 4,5 and 6 is a lot of unneeded - // bookkeeping redcheck(sibling_color) { - // case 3 + // Case 3 if (parent->parent) { parent->rotate(dir); } else { @@ -801,6 +800,14 @@ private: sibling = close; distant = sibling->child(!dir); + // Checking after conditions + // GOTO is used due to heavy sharing of state between + // cases which would be annoying to pass as procedure + // argument(s) especially for handling the various places + // cases 4, 5 and 6 show up at + // In theory this could've been better handled as an + // explicit FSM object but it was easier to do this via + // GOTO in the moment if (distant != nullptr && distant->color == Color::Red) { goto case_6; } @@ -811,7 +818,6 @@ private: goto case_4; } else redcheck(close_color) { - // case 5 case_5: assert(sibling); assert(close); @@ -823,7 +829,6 @@ private: goto case_6; } else redcheck(distant_color) { - // case 6 case_6: assert(parent); assert(sibling); @@ -840,7 +845,6 @@ private: return; } else redcheck(par_color) { - // case 4 case_4: assert(sibling); sibling->color = Color::Red; @@ -848,12 +852,22 @@ private: return; } else { - // case 2 + // Case 2: we need to keep cranking through + // the motions backtracking up the tree so we + // reassign target appropriately after recoloring assert(sibling); sibling->color = Color::Red; - n = parent; - continue; + target = parent; } + // Case 1: After backtracking up the tree we may reach the root + // node in which case we've finished resetting our invariants + // if we aren't at the root node this code will update parent + // and dir to appropriate values + parent = target->parent; + if (parent == nullptr) { + return; + } + dir = parent->which_child(target); } }