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

Add priority queue #2008

Merged
merged 1 commit into from
Feb 26, 2019
Merged

Add priority queue #2008

merged 1 commit into from
Feb 26, 2019

Conversation

schmee
Copy link
Contributor

@schmee schmee commented Feb 25, 2019

This PR adds a simple heap-based priority queue to the standard library. I made this while working on #213, and based on this comment I thought that I might as well shoot a PR and see if it is useful for anyone else. I've tried to emulate the conventions in ArrayList and HashMap as much as I could.

Some outstanding API design questions:

  • Should there be an addAssumeCapacity method?
  • Should there be a peek / peekOrNull combo?
  • Should shrink also reallocate the underlying array?
    • ArrayList.shrink doesn't and that is a bit surprising to me
  • Should we have a modification_count like HashMap to detect modifications during iteration?
    • HashMap has it but ArrayList doesn't, it would be nice with consistency here. I can make a PR for adding it to ArrayList if people think it's a good idea.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Looks good, and well tested. Much appreciated. I didn't inspect the algorithm itself but I'll give it a closer look before merging. I'll leave this open for about a day in case any other contributors want to weigh in before merging.

As to your questions:

Should there be an addAssumeCapacity method?
Should there be a peek / peekOrNull combo?

Let's start with what you have. If it becomes clear there is a need for these we can add them. Also a reminder, there will be a Comprehensive Standard Library Audit before 1.0.0 in which we'll have a chance to standardize these things.

Should shrink also reallocate the underlying array?

  • ArrayList.shrink doesn't and that is a bit surprising to me

This is a question that needs a deeper investigation. I'll open an issue for this, but the quick summary is: I would like it to call shrink in the allocator. However the allocator currently has no way to communicate back, "hey I actually can't reclaim this memory, so if you just want to hang on to it, that would actually be better". So I want to look into this before deciding how shrink will work for array lists. I think you can avoid this issue for now and we'll make sure to come back and audit priority queue when this issue is resolved.

Edit: opened #2009

Should we have a modification_count like HashMap to detect modifications during iteration?

  • HashMap has it but ArrayList doesn't, it would be nice with consistency here. I can make a PR for adding it to ArrayList if people think it's a good idea.

I think the debug runtime safety is worthwhile if and only if the iterator is in danger of becoming invalid when the data structure is modified. I'm not sure why ArrayList even has an Iterator API, but anyway, the iterator just has an index; modifications to the ArrayList would not cause the iterator to become invalid.

Hope that helps, and thanks for the contribution!

@emekoi
Copy link
Contributor

emekoi commented Feb 26, 2019

shouldn't the tests adopt a common naming convention or prefix so things like --test-filter work for selecting these tests?

@schmee
Copy link
Contributor Author

schmee commented Feb 26, 2019

Thanks for the feedback everyone!

@emekoi Sounds good, I'll update the PR 👍

@schmee
Copy link
Contributor Author

schmee commented Feb 26, 2019

Added std.PriorityQueue as the test prefix, will add the fromSlice method in another PR cause I want to think a bit more about how to do it. So this PR should be good to go 👍

@andrewrk andrewrk merged commit 0eed72d into ziglang:master Feb 26, 2019
@schmee schmee deleted the priority-queue branch February 26, 2019 18:43
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.

4 participants