Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in split_before and split_after for first and last node edge cases #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

morlinbrot
Copy link

Fixes #18

The split_before and split_after methods don't handle the edge case of being at the first or last node correctly.

Example split_before:
In the current implementation, output_front will be set to the list's head before checking if there actually is a node before where the cursor currently is, which in the case of being at the first node will not be the case. In this case, we want to return an empty list which can be done by simply not setting a new head.

I tried to implement a fix as unobtrusive to the original implementation as possible, since I assume that the methods are written with readability in mind for the too-many-lists project. In my implementation, I wrote the methods a bit more "cleverly" which may be not as easy to follow but shorter and more focused on all the edge cases that need to be covered, here's a snippet of it:

pub fn split_before(&mut self) -> LinkedList<T> {
    match self.idx {
        None => mem::replace(self.list, LinkedList::new()),
        Some(0) => LinkedList::new(),
        Some(idx) => unsafe {
            let cur = self.cur.unwrap();
            let out_head = self.list.head;
            let out_tail = (*cur.as_ptr()).prev.take();

            if let Some(node) = out_tail {
                (*node.as_ptr()).next = None;
            }

            let old_len = self.list.len;
            let new_len = old_len - idx;

            self.list.head = Some(cur);
            self.list.len = new_len;
            self.idx = Some(0);

            LinkedList {
                head: out_head,
                tail: out_tail,
                len: old_len - new_len,
                _marker: PhantomData,
            }
        },
    }
}

Let me know if you'd be interested in switching to my implementation, I'd be glad to update the PR.

@morlinbrot
Copy link
Author

Note the differences in implementation between this and the same fix over at too-many-lists.

@alexkudryavtsev88
Copy link

alexkudryavtsev88 commented May 11, 2024

I've checked your new solution - it looks like all stuff works fine:

  • cursor.split_before() after first cursor.move_next() returns empty list, the source list stays in initial state;
  • also, there is no more attempt to subtract with overflow panic at pop_front that's called upon the list drop (this issue was present in "book" implementation)

@pczarn
Copy link
Contributor

pczarn commented Dec 2, 2024

I'll look into this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: split_before and split_after return malformed list when cursor is at first or last node
3 participants