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

feat(stdlib)!: Make queue and stack mutable & provide Immutable submodules #1479

Merged
merged 7 commits into from
Feb 22, 2023

Conversation

alex-snezhko
Copy link
Member

Mutable array-based implementations for queues and stacks. From a few benchmarks these implementations offer much faster performance than the current immutable Queue and Stack implementations (likely due to gains in cache locality because of underlying array representation), which may make them more viable for performance-sensitive applications.

@phated phated requested a review from a team November 28, 2022 23:45
@phated
Copy link
Member

phated commented Nov 30, 2022

@alex-snezhko I think we're going to hold this PR back from 0.5.5 because we think it is confusing to have Queue and Stack be the immutable versions but Map & Set are mutable (and then have the inverse have the special naming). We're going to deprecated the immutable Queue & Stack in 0.5.5 and then have you make this the breaking change that moves stuff around.

How does that sound?

@alex-snezhko
Copy link
Member Author

Sounds good!

@phated phated changed the title feat(stdlib): Mutable queue and stack feat(stdlib)!: Mutable queue and stack Dec 1, 2022
Copy link
Member

@marcusroberts marcusroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy to read and clear code, great documentation and tests.

Looks very good to me!

@alex-snezhko
Copy link
Member Author

#1644 and #1621 should probably be resolved before merging this

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-snezhko I've rebased this so I could generate the docs with my submodule support in #1684 - however, I think you need to update the @since and @history for the various things that moved around. e.g. Since functions moved from the root into the Immutable submodule, I think those should be @since v0.6.0 with a history item saying they were originally a root API (similar to how we did those other PRs).

@phated
Copy link
Member

phated commented Feb 21, 2023

Rebased again and regenerated the docs with graindoc fixes.

@phated phated changed the title feat(stdlib)!: Mutable queue and stack feat(stdlib)!: Make queue and stack mutable & provide Immutable submodules Feb 21, 2023
Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great! Would just love if these record fields were renamed.

stdlib/queue.gr Outdated Show resolved Hide resolved
@ospencer
Copy link
Member

Also looks like the set tests need to be formatted.

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change Oscar wanted, so this should be good to queue.

@phated phated added this pull request to the merge queue Feb 22, 2023
@alex-snezhko
Copy link
Member Author

Thanks @phated :)

Merged via the queue into grain-lang:main with commit 979a20c Feb 22, 2023
av8ta pushed a commit to av8ta/grain that referenced this pull request Apr 11, 2023
…dules (grain-lang#1479)

* feat(stdlib): Mutable queue and stack

* moved mutable impls to root of modules

* generate submodule docs with graindoc submodule support

* updated history

* update docs with graindoc fixes

* formatting

* update names as per review

---------

Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants