Fix/splaytree delete last node - #366
Open
B7M wants to merge 359 commits into
Open
Conversation
Allows pressing stepback and stepforward during an animation, and to go from the complete start and complete end of the animations without the steps stack being lost or requiring a re-run of the simulation
Add Create Graph functionality
fixed finals banner
Adjusted UI for better viewing
Scroll viewpoint
fixed large graph
corrected singly linked list code
Fix indentation for StackArray.push()
…wording Fix wording for quickSelect pseudo
Making csvistool more accessible
treeDelete reassigns treeRoot to its left child and then writes treeRoot.parent unconditionally. With one node in the tree both children are null, so treeRoot becomes null and the write throws. The throw escapes before startNewAnimation, dropping the queued act.delete, so the deleted node stays drawn on the canvas. Guard the parent write; resizeTree already handles a null root.
|
@B7M is attempting to deploy a commit to the rodrigodlpontes' projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix crash when deleting the last remaining node in a Splay Tree
---------- PR DESCRIPTION BELOW THIS LINE ----------
Problem
In
SplayTree.treeDelete, the root is replaced by its left child and then written to right away:When there is only one node in the tree, that node has no left child and no right child. The code takes this branch because the right child is
null, sothis.treeRootbecomesnulltoo. The next line then tries to setparentonnulland throws:The error also stops the animation commands for that delete from running, and one of those commands is the one that erases the node's circle. So the node the user just deleted is still drawn on the screen, even though the tree is now empty. The two stay out of sync until you press
Clear.This is not only the one-node case. Any sequence that empties the tree hits it, because the last delete always happens when a single node is left.
Fix
I checked for
nullbefore settingparent:The tree is allowed to be empty here, so a
nullroot is a normal result and not an error.resizeTree()already checks for anullroot, so nothing else needed to change.I did not change the other branch (
left == null). That one only runs when there is a right child, so its new root can never benull.Testing
I tested the change in the browser and looked at what was actually drawn on the canvas at the end.
nodesbelow is what stays on the screen.5, delete5nodes=[5]❌nodes=[]✅5, delete5, insert7nodes=[5,7]❌nodes=[7]✅5, delete5, insert7, delete7nodes=[5,7]❌nodes=[]✅5,3, delete3, delete5nodes=[5]❌nodes=[]✅5,3,8, delete3,5,8nodes=[8]❌nodes=[]✅5, delete5, insert5,2,9nodes=[5,5,2,9]❌nodes=[5,2,9]✅The last row shows the leftover node most clearly: the old
5is still there, so the canvas draws5twice.I also checked the cases that were already working, to make sure the fix did not change them. These behave the same before and after:
5,3, delete5(root with one child)nodes=[3], no error5,3,8, delete5(root with two children)nodes=[3,8], no error5, delete9(value not in the tree)nodes=[5], no error9from an empty treenodes=[], no errorTo see the bug you have to delete the tree down to empty by hand, which is probably why it went unnoticed — the
Randombutton only inserts, it never deletes.Checks
npm run lintpassedsrc/algo/SplayTree.jswas changed (3 lines added, 1 removed)