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

BinaryHeap::retain would be useful #50671

Closed
Yoric opened this issue May 11, 2018 · 7 comments
Closed

BinaryHeap::retain would be useful #50671

Yoric opened this issue May 11, 2018 · 7 comments
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Yoric
Copy link
Contributor

Yoric commented May 11, 2018

Consider a BinaryHeap in which we need to change priority of some elements. For the moment, this only way to implement this is to drain the heap and rebuild it from scratch. It would be more efficient to be able to remove only the affected elements, so as to read them.

A variant would be a

/// If `f(foo)` returns `Some(bar)`, replace `foo` with `bar`, which may have a different priority.
fn replace<F>(&mut self, f: F) where F: FnMut(&T) -> Option<T>;
@sfackler
Copy link
Member

FnMut(&mut T) -> bool matches up with other retain signatures and is more flexible. Seems like a reasonable method to add though!

@Yoric
Copy link
Contributor Author

Yoric commented May 12, 2018

If it's FnMut(&mut T) -> bool (which is fine by me), the closure can cause every element to change weight, so it will require a full reordering/rebalancing. Probably ok, but complexity needs to be checked.

@pmarcelll
Copy link
Contributor

There was a discussion on the C++ subreddit recently that is related to this topic, it might be a good place to look for ideas.

@sfackler
Copy link
Member

Ah right, I didn't see that signature was for replace rather than retain. I think replace would still want something that can update the original value in place, but it's not so clear to me what that'd be.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-feature-accepted Category: A feature request that has been accepted pending implementation. labels May 29, 2018
@Mark-Simulacrum
Copy link
Member

Closing in favor of #42849.

@pmarcelll
Copy link
Contributor

@Mark-Simulacrum This issue is for BinaryHeap, and #42849 is for BTreeSet/BTreeMap. If this issue remains closed, it would be a good idea to modify the other issue to include BinaryHeap as well.

@Mark-Simulacrum
Copy link
Member

Thanks, included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants