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

Implement Clone::clone_from for VecDeque #65069

Merged
merged 3 commits into from
Oct 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 99 additions & 2 deletions src/liballoc/collections/vec_deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
use core::array::LengthAtMost32;
use core::cmp::{self, Ordering};
use core::fmt;
use core::iter::{repeat_with, FromIterator, FusedIterator};
use core::mem;
use core::iter::{once, repeat_with, FromIterator, FusedIterator};
use core::mem::{self, replace};
use core::ops::Bound::{Excluded, Included, Unbounded};
use core::ops::{Index, IndexMut, RangeBounds, Try};
use core::ptr::{self, NonNull};
Expand Down Expand Up @@ -57,11 +57,88 @@ pub struct VecDeque<T> {
buf: RawVec<T>,
}

/// PairSlices pairs up equal length slice parts of two deques
///
/// For example, given deques "A" and "B" with the following division into slices:
///
/// A: [0 1 2] [3 4 5]
/// B: [a b] [c d e]
///
/// It produces the following sequence of matching slices:
///
/// ([0 1], [a b])
/// ([2], [c])
/// ([3 4], [d e])
///
/// and the uneven remainder of either A or B is skipped.
struct PairSlices<'a, 'b, T> {
crgl marked this conversation as resolved.
Show resolved Hide resolved
a0: &'a mut [T],
a1: &'a mut [T],
b0: &'b [T],
b1: &'b [T],
}

impl<'a, 'b, T> PairSlices<'a, 'b, T> {
fn from(to: &'a mut VecDeque<T>, from: &'b VecDeque<T>) -> Self {
let (a0, a1) = to.as_mut_slices();
let (b0, b1) = from.as_slices();
PairSlices { a0, a1, b0, b1 }
}

fn has_remainder(&self) -> bool {
!self.b0.is_empty()
}

fn remainder(self) -> impl Iterator<Item=&'b [T]> {
once(self.b0).chain(once(self.b1))
}
}

impl<'a, 'b, T> Iterator for PairSlices<'a, 'b, T>
{
type Item = (&'a mut [T], &'b [T]);
fn next(&mut self) -> Option<Self::Item> {
// Get next part length
let part = cmp::min(self.a0.len(), self.b0.len());
if part == 0 {
return None;
}
let (p0, p1) = replace(&mut self.a0, &mut []).split_at_mut(part);
let (q0, q1) = self.b0.split_at(part);

// Move a1 into a0, if it's empty (and b1, b0 the same way).
self.a0 = p1;
self.b0 = q1;
if self.a0.is_empty() {
self.a0 = replace(&mut self.a1, &mut []);
}
if self.b0.is_empty() {
self.b0 = replace(&mut self.b1, &[]);
}
Some((p0, q0))
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: Clone> Clone for VecDeque<T> {
fn clone(&self) -> VecDeque<T> {
self.iter().cloned().collect()
}

fn clone_from(&mut self, other: &Self) {
self.truncate(other.len());

let mut iter = PairSlices::from(self, other);
while let Some((dst, src)) = iter.next() {
dst.clone_from_slice(&src);
}
scottmcm marked this conversation as resolved.
Show resolved Hide resolved

if iter.has_remainder() {
for remainder in iter.remainder() {
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to put a reserve here before the extends. That said, with only 2 extend calls, it's not super necessary — if extend would reserve at all, but it doesn't(!). So extend is missing that first order optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to change extend to use reserve (separately), or is it just standard practice to use reserve before extend? The only other thing is that conditionally reserving at the beginning of clone_from would prevent any work from being done in the OOM case, which could be a feature.

In general, though, it seems like reserving before extending makes sense since every iterator provides a lower bound through size_hint, some common cases are exact, and calling extend more than a few times should be rare.

Copy link
Member

Choose a reason for hiding this comment

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

Extend should in general issue the reserve itself, here it is known it will grow by at least the lower bound, so it should use that. (It can use other optimizations like TrustedLen later, like Vec does.)

self.extend(remainder.iter().cloned());
}
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -2209,6 +2286,16 @@ impl<'a, T> Iterator for Iter<'a, T> {
final_res
}

fn nth(&mut self, n: usize) -> Option<Self::Item> {
if n >= count(self.tail, self.head, self.ring.len()) {
self.tail = self.head;
None
} else {
self.tail = wrap_index(self.tail.wrapping_add(n), self.ring.len());
self.next()
}
}

#[inline]
fn last(mut self) -> Option<&'a T> {
self.next_back()
Expand Down Expand Up @@ -2327,6 +2414,16 @@ impl<'a, T> Iterator for IterMut<'a, T> {
back.iter_mut().fold(accum, &mut f)
}

fn nth(&mut self, n: usize) -> Option<Self::Item> {
if n >= count(self.tail, self.head, self.ring.len()) {
self.tail = self.head;
None
} else {
self.tail = wrap_index(self.tail.wrapping_add(n), self.ring.len());
self.next()
}
}

#[inline]
fn last(mut self) -> Option<&'a mut T> {
self.next_back()
Expand Down
23 changes: 23 additions & 0 deletions src/liballoc/collections/vec_deque/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,29 @@ fn test_vec_from_vecdeque() {
}
}

#[test]
fn test_clone_from() {
let m = vec![1; 8];
let n = vec![2; 12];
for pfv in 0..8 {
for pfu in 0..8 {
for longer in 0..2 {
let (vr, ur) = if longer == 0 { (&m, &n) } else { (&n, &m) };
let mut v = VecDeque::from(vr.clone());
for _ in 0..pfv {
v.push_front(1);
}
let mut u = VecDeque::from(ur.clone());
for _ in 0..pfu {
u.push_front(2);
}
v.clone_from(&u);
assert_eq!(&v, &u);
}
}
}
}

#[test]
fn issue_53529() {
use crate::boxed::Box;
Expand Down