Skip to content

Add PeekMut::pop #38733

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

Merged
merged 2 commits into from
Jan 7, 2017
Merged

Add PeekMut::pop #38733

merged 2 commits into from
Jan 7, 2017

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Dec 31, 2016

A fairly common workflow is to put a bunch of stuff into a binary heap
and then mutate the top value until its empty. This both makes that a
bit more convenient (no need to save a boolean off and pop after to
avoid borrowck issues), and a bit more efficient since you only shift
once.

r? @alexcrichton

cc @rust-lang/libs

@alexcrichton
Copy link
Member

Sounds reasonable to me! Could you add a comment on the struct definition of PeekMut though indicating that it's intended to be leaked, and therefore shouldn't contain any allocations? (just to try to future-proof ourselves)

@alexcrichton
Copy link
Member

Or better yet I wouldn't mind a boolean flag to manage this.

A fairly common workflow is to put a bunch of stuff into a binary heap
and then mutate the top value until its empty. This both makes that a
bit more convenient (no need to save a boolean off and pop after to
avoid borrowck issues), and a bit more efficient since you only shift
once.
@sfackler
Copy link
Member Author

sfackler commented Jan 2, 2017

Updated

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 2, 2017
@alexcrichton
Copy link
Member

Alright let's merge!

@sfackler r=me with a tracking issue updated

@sfackler
Copy link
Member Author

sfackler commented Jan 6, 2017

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Jan 6, 2017

📌 Commit 54dc533 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Jan 7, 2017

⌛ Testing commit 54dc533 with merge b97b605...

bors added a commit that referenced this pull request Jan 7, 2017
Add PeekMut::pop

A fairly common workflow is to put a bunch of stuff into a binary heap
and then mutate the top value until its empty. This both makes that a
bit more convenient (no need to save a boolean off and pop after to
avoid borrowck issues), and a bit more efficient since you only shift
once.

r? @alexcrichton

cc @rust-lang/libs
@bors
Copy link
Collaborator

bors commented Jan 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b97b605 to master...

@bors bors merged commit 54dc533 into rust-lang:master Jan 7, 2017
@arthurprs
Copy link
Contributor

Any reason for making it a static function instead of a member function that takes Self? The later is shorter and doesn't require importing.

@sfackler
Copy link
Member Author

PeekMut implements Deref so a non-static pop method would shadow the pop method of the wrapped type. We do the same for methods on Rc, Box, etc.

@sfackler sfackler deleted the peek-mut-pop branch January 11, 2017 17:01
@arthurprs
Copy link
Contributor

Got it, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants