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 MutableLinkedList when appending empty list #915

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

Conversation

yohmercy
Copy link

@yohmercy yohmercy commented Oct 5, 2024

Currently append!(MutableLinkedList{Int}(1), MutableLinkedList{Int}()) results in an infinite iteration. This is due to the order of sentinel-node reassignments and the second list being empty. One way to fix that is by using something like isempty(l2) && return l1 at the top of the append! method. Another (proposed in this PR) is to just reorder the reassignments. The former is more straightforward, the latter adds no new code.

@yohmercy
Copy link
Author

yohmercy commented Oct 15, 2024

@oxinabox I apologize for tagging you directly: I'm new to contributing, so I'm not sure if that's OK. The changes here are minimal, yet having an infinite iteration every time I try to append an empty list is troublesome. I resort to using my own local version of the package in the meantime. Would you mind reviewing the PR? I'm not sure which of the two options (if any) is better. Thank you.

@oxinabox
Copy link
Member

tagging me is fine.
I am very busy right now.
I will try and find time for this next week.
If another maintainer doesn't come along and look first.

In general I hate the linked list code and want to just delete all of it.
In almost no real world senario is a linked list the right datastructure to use

@fingolfin
Copy link
Member

@yohmercy could you perhaps add a test that demonstrates the bug fix works (i.e. something that fails now and works correctly with your patch)

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.

3 participants