add VecDeque::splice #147247
add VecDeque::splice #147247
Conversation
rustbot
added
S-waiting-on-review
|
Thanks for working on this! |
|
Sorry I haven't had a chance to look at this, still behind on my reviews r? libs |
|
☔ The latest upstream changes (presumably #148337) made this pull request unmergeable. Please resolve the merge conflicts. |
| return; | ||
| } | ||
|
|
||
| if !self.drain.fill(&mut self.replace_with) { |
There was a problem hiding this comment.
This would invalidate source_deque -- can we avoid accidental mis-use by removing that variable, inlining the as_mut() above?
| if true { | ||
| // if !self.drain.fill(tail_start, &mut self.replace_with) { |
There was a problem hiding this comment.
Why this change/comment?
| if let Some(new_item) = replace_with.next() { | ||
| let index = unsafe { self.deque.as_ref() }.to_physical_idx(idx); | ||
| unsafe { self.deque.as_mut().buffer_write(index, new_item) }; | ||
| deque.len += 1; |
There was a problem hiding this comment.
I'd expect this to cause UB -- self.deque.as_mut() has invalidated the earlier deque &mut already at this point?
| // This will set drain.remaining to 0, so its drop won't try to read deallocated memory on | ||
| // drop. | ||
| self.drain.by_ref().for_each(drop); | ||
|
|
There was a problem hiding this comment.
Do we need something like
rust/library/alloc/src/vec/splice.rs
Line 62 in 87f9dcd
| impl<I: Iterator, A: Allocator> ExactSizeIterator for Splice<'_, I, A> {} | ||
|
|
||
| #[unstable(feature = "deque_extend_front", issue = "146975")] | ||
| impl<I: Iterator, A: Allocator> Drop for Splice<'_, I, A> { |
There was a problem hiding this comment.
Can you add a comment here and in Vec's Splice pointing at each other?
| return; | ||
| } | ||
|
|
||
| if !self.drain.fill(&mut self.replace_with) { |
There was a problem hiding this comment.
Also, can you restore the comment from Vec ("First fill the range left by drain().")?
| /// have been moved out. | ||
| /// Fill that range as much as possible with new elements from the `replace_with` iterator. | ||
| /// Returns `true` if we filled the entire range. (`replace_with.next()` didn’t return `None`.) | ||
| unsafe fn fill<I: Iterator<Item = T>>(&mut self, replace_with: &mut I) -> bool { |
There was a problem hiding this comment.
What's the safety precondition on this method?
| /// This is optimal if: | ||
| /// | ||
| /// * The tail (elements in the deque after `range`) is empty, | ||
| /// * or `replace_with` yields fewer or equal elements than `range`'s length | ||
| /// * or the lower bound of its `size_hint()` is exact. | ||
| /// | ||
| /// Otherwise, a temporary vector is allocated and the tail is moved twice. |
There was a problem hiding this comment.
Can we add a note to the tracking issue to confirm this is accurate? I'm not sure but it feels like there ought to be slightly different conditions vs. Vec, since we have a potentially 'empty' middle to play with...
| unsafe fn move_tail(&mut self, additional: usize) { | ||
| let deque = unsafe { self.deque.as_mut() }; | ||
| let tail_start = deque.len + self.drain_len; | ||
| deque.buf.reserve(tail_start + self.tail_len, additional); |
There was a problem hiding this comment.
Do we have coverage for this reserve running? Normally growing VecDeque requires extra logic AFAICT, not just reserving the underlying buffer.
rustbot
added
S-waiting-on-author
6 participants
Tracking issue: #146975