Implement placement-in protocol for BinaryHeap
#39062
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| impl<'a, T> Place<T> for PlaceIn<'a, T> | ||
| where T: Clone + Ord { | ||
| fn pointer(&mut self) -> *mut T { | ||
| self.heap.data.place_back().pointer() |
There was a problem hiding this comment.
This seems… incorrect. Read the comment for Placer.
|
|
||
| fn make_place(self) -> Self { | ||
| let _ = self.heap.data.place_back().make_place(); | ||
| self |
There was a problem hiding this comment.
Implementation of this is going in the right direction but not quite there yet.
Probably the most notable issue here is that the Place that you create in the underlying vector gets immediatelly dropped, after which you are not allowed to put anything into the place in question anymore.
While this may work in current implementation, because PlaceBack for Vec has no Drop defined, consider what would happen if it had one:
// self.heap.data here is just big enough that a call to make_place would resize the vector.
let place = self.heap.data.place_back().make_place(); // resizes vector
drop(place); // resizes the vector back to the original size if was implemented weirdly
// now the placement protocol asks for a pointer to put data in
self.heap.data.place_back().pointer() // returns a pointer out of bounds!This implementation therefore does not follow the placement protocol.
What ought be done instead is a new structure such as this (I think, feel free to explore alternative designs! generics omitted for brevity):
struct BinaryHeapPlace {
heap: &mut BinaryHeap
place: vec::PlaceBack
}and then use the place in this structure to produce the pointer and also call the finalize on.
There was a problem hiding this comment.
I tried this initially, but quickly ran into problems, as vec::PlaceBack borrows the vec its created from. Might there be some clever way of bypassing this problem? I would like to reuse vec::PlaceBack somehow, as it does exactly what I try to do, and just throw in the heap.sift_up in InPlace::finalize, but it seems to be hard wrt. borrowing and aliasing stuff.
| let len = self.heap.len(); | ||
| let _ = self.heap.data.place_back().finalize(); | ||
| let i = self.heap.sift_up_ind(0, len); | ||
| &mut self.heap.data[i] |
There was a problem hiding this comment.
Returning an immutable reference seems sensible, but you needn’t to do a &mut self.heap.data[i] here; while return value of finalize cannot be reused, you can still avoid a bounds check by simply doing self.heap.get_unchecked(i), as the index is guaranteed to be in bounds.
| @@ -688,6 +688,22 @@ impl<T: Ord> BinaryHeap<T> { | |||
| } | |||
| } | |||
|
|
|||
| fn sift_up_ind(&mut self, start: usize, pos: usize) -> usize { | |||
There was a problem hiding this comment.
You can simply adjust the sift_up to return the index, since its a private implementation detail and will also avoid the code duplication.
| unsafe { | ||
| // Take out the value at `pos` and create a hole. | ||
| let mut hole = Hole::new(&mut self.data, pos); | ||
|
|
||
| while hole.pos() > start { | ||
| let parent = (hole.pos() - 1) / 2; | ||
| if hole.element() <= hole.get(parent) { | ||
| break; | ||
| return hole.pos(); |
There was a problem hiding this comment.
Ops, I think I'll revert that 😄
martinhath
force-pushed
the
placement-in-binaryheap
branch
from
ef856ee to
90fbe15
Compare
|
r? @nagisa (who looks like is definitely still willing to mentor!) |
|
Er, @nagisa feel free to ping/cc me when this is ready and I'll r=you |
|
This impl looks great to me. Certainly r+ from me. |
|
@bors: r=nagisa |
|
📌 Commit 90fbe15 has been approved by |
alexcrichton
added
the
T-libs-api
Implement placement-in protocol for `BinaryHeap` Related to #30172, and loosley based on #38551. At the moment, this PR is in a pretty rough state, but I wanted to get some feedback to see if I'm going in the right direction. I hope the Mentor label of #30172 is still applicable, even though it's a year old 😄
|
☀️ Test successful - status-appveyor, status-travis |
Rollup of 28 pull requests - Successful merges: #38603, #38761, #38842, #38847, #38955, #38966, #39062, #39068, #39077, #39111, #39112, #39114, #39118, #39120, #39132, #39135, #39138, #39142, #39143, #39146, #39157, #39166, #39167, #39168, #39179, #39184, #39195, #39197 - Failed merges: #39060, #39145
Rollup of 28 pull requests - Successful merges: #38603, #38761, #38842, #38847, #38955, #38966, #39062, #39068, #39077, #39111, #39112, #39114, #39118, #39120, #39132, #39135, #39138, #39142, #39143, #39146, #39157, #39166, #39167, #39168, #39179, #39184, #39195, #39197 - Failed merges: #39060, #39145
Related to #30172, and loosley based on #38551.
At the moment, this PR is in a pretty rough state, but I wanted to get some feedback to see if I'm going in the right direction.
I hope the Mentor label of #30172 is still applicable, even though it's a year old 😄