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

Make LinkedList a Union instead of Abstract type #877

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arikheinss
Copy link

Hello. I have a proposal for an improvement of the implementation of the linked list data structure.

The way the LinkedList is implemented right now LinkedList is an abstract datatype, and Cons contains a field of type LinkedList. This means that any operation on LinkedLists result in a ton of unavoidable runtime-dispatch.

I noticed that a small redesign where Nil and Cons are standalone types and LinkedList just refers to their Union provides a speedup of more than an order of magnitude, as the compiler can create very efficient code for simple Unions of a concrete and a singleton type. The only disadvantage I see in this is that users can no longer extend the LinkedList type with new types, but it may be argued that this would not be intended behavior anyway.
I also made the two types immutable. They used to be mutable, but the mutability was never utilized. If there is some reason to make these mutable (like forcing the compiler to put them on the heap) let me know.

Made Nil and Cons standalone immutable types, with LilnkedList being the Union of the two
@arikheinss
Copy link
Author

arikheinss commented Oct 29, 2023

I am sorry, I dont do much stuff on git. The second commit is supposed to go into it's own PR, but ended up being here somehow.

The second commit is a fix to the stackOverflow problem described in issue #878. It should provide value independently of whether the first commit is merged or not.

Everything for that juicy code coverage
@arikheinss
Copy link
Author

Added a test for better code coverage while we are at it

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.

None yet

1 participant