Skip to content

Commit ecca8c5

Browse files
Sp00phSp00ph
committed
Changes according to code review
1 parent a1bf25e commit ecca8c5

File tree

6 files changed

+181
-144
lines changed

6 files changed

+181
-144
lines changed

‎library/alloc/src/collections/vec_deque/drain.rs

+46-33
Original file line numberDiff line numberDiff line change
@@ -53,27 +53,36 @@ impl<'a, T, A: Allocator> Drain<'a, T, A> {
5353
}
5454

5555
// Only returns pointers to the slices, as that's
56-
// all we need to drop them
57-
fn as_slices(&self) -> (*mut [T], *mut [T]) {
56+
// all we need to drop them. May only be called if `self.remaining != 0`.
57+
unsafe fn as_slices(&self) -> (*mut [T], *mut [T]) {
5858
unsafe {
5959
let deque = self.deque.as_ref();
60-
let wrapped_start = deque.wrap_idx(self.idx);
61-
62-
if self.remaining <= deque.capacity() - wrapped_start {
63-
// there's only one contigous slice
64-
(
65-
ptr::slice_from_raw_parts_mut(deque.ptr().add(wrapped_start), self.remaining),
66-
&mut [] as *mut [T],
67-
)
60+
// FIXME: This is doing almost exactly the same thing as the else branch in `VecDeque::slice_ranges`.
61+
// Unfortunately, we can't just call `slice_ranges` here, as the deque's `len` is currently
62+
// just `drain_start`, so the range check would (almost) always panic. Between temporarily
63+
// adjusting the deques `len` to call `slice_ranges`, and just copy pasting the `slice_ranges`
64+
// implementation, this seemed like the less hacky solution, though it might be good to
65+
// find a better one in the future.
66+
67+
// because `self.remaining != 0`, we know that `self.idx < deque.original_len`, so it's a valid
68+
// logical index.
69+
let wrapped_start = deque.to_physical_idx(self.idx);
70+
71+
let head_len = deque.capacity() - wrapped_start;
72+
73+
let (a_range, b_range) = if head_len >= self.remaining {
74+
(wrapped_start..wrapped_start + self.remaining, 0..0)
6875
} else {
69-
let head_len = deque.capacity() - wrapped_start;
70-
// this will never overflow due to the if condition
7176
let tail_len = self.remaining - head_len;
72-
(
73-
ptr::slice_from_raw_parts_mut(deque.ptr().add(wrapped_start), head_len),
74-
ptr::slice_from_raw_parts_mut(deque.ptr(), tail_len),
75-
)
76-
}
77+
(wrapped_start..deque.capacity(), 0..tail_len)
78+
};
79+
80+
// SAFETY: the range `self.idx..self.idx+self.remaining` lies strictly inside
81+
// the range `0..deque.original_len`. because of this, and because of the fact
82+
// that we acquire `a_range` and `b_range` exactly like `slice_ranges` would,
83+
// it's guaranteed that `a_range` and `b_range` represent valid ranges into
84+
// the deques buffer.
85+
(deque.buffer_range(a_range), deque.buffer_range(b_range))
7786
}
7887
}
7988
}
@@ -103,8 +112,9 @@ impl<T, A: Allocator> Drop for Drain<'_, T, A> {
103112
impl<'r, 'a, T, A: Allocator> Drop for DropGuard<'r, 'a, T, A> {
104113
fn drop(&mut self) {
105114
if self.0.remaining != 0 {
106-
let (front, back) = self.0.as_slices();
107115
unsafe {
116+
// SAFETY: We just checked that `self.remaining != 0`.
117+
let (front, back) = self.0.as_slices();
108118
ptr::drop_in_place(front);
109119
ptr::drop_in_place(back);
110120
}
@@ -133,7 +143,7 @@ impl<T, A: Allocator> Drop for Drain<'_, T, A> {
133143
source_deque.len = 0;
134144
}
135145
(0, _) => {
136-
source_deque.head = source_deque.wrap_idx(drain_len);
146+
source_deque.head = source_deque.to_physical_idx(drain_len);
137147
source_deque.len = orig_len - drain_len;
138148
}
139149
(_, 0) => {
@@ -143,15 +153,15 @@ impl<T, A: Allocator> Drop for Drain<'_, T, A> {
143153
if head_len <= tail_len {
144154
source_deque.wrap_copy(
145155
source_deque.head,
146-
source_deque.wrap_idx(drain_len),
156+
source_deque.to_physical_idx(drain_len),
147157
head_len,
148158
);
149-
source_deque.head = source_deque.wrap_idx(drain_len);
159+
source_deque.head = source_deque.to_physical_idx(drain_len);
150160
source_deque.len = orig_len - drain_len;
151161
} else {
152162
source_deque.wrap_copy(
153-
source_deque.wrap_idx(head_len + drain_len),
154-
source_deque.wrap_idx(head_len),
163+
source_deque.to_physical_idx(head_len + drain_len),
164+
source_deque.to_physical_idx(head_len),
155165
tail_len,
156166
);
157167
source_deque.len = orig_len - drain_len;
@@ -162,14 +172,17 @@ impl<T, A: Allocator> Drop for Drain<'_, T, A> {
162172
}
163173

164174
let guard = DropGuard(self);
165-
let (front, back) = guard.0.as_slices();
166-
unsafe {
167-
// since idx is a logical index, we don't need to worry about wrapping.
168-
guard.0.idx += front.len();
169-
guard.0.remaining -= front.len();
170-
ptr::drop_in_place(front);
171-
guard.0.remaining = 0;
172-
ptr::drop_in_place(back);
175+
if guard.0.remaining != 0 {
176+
unsafe {
177+
// SAFETY: We just checked that `self.remaining != 0`.
178+
let (front, back) = guard.0.as_slices();
179+
// since idx is a logical index, we don't need to worry about wrapping.
180+
guard.0.idx += front.len();
181+
guard.0.remaining -= front.len();
182+
ptr::drop_in_place(front);
183+
guard.0.remaining = 0;
184+
ptr::drop_in_place(back);
185+
}
173186
}
174187

175188
// Dropping `guard` handles moving the remaining elements into place.
@@ -185,7 +198,7 @@ impl<T, A: Allocator> Iterator for Drain<'_, T, A> {
185198
if self.remaining == 0 {
186199
return None;
187200
}
188-
let wrapped_idx = unsafe { self.deque.as_ref().wrap_idx(self.idx) };
201+
let wrapped_idx = unsafe { self.deque.as_ref().to_physical_idx(self.idx) };
189202
self.idx += 1;
190203
self.remaining -= 1;
191204
Some(unsafe { self.deque.as_mut().buffer_read(wrapped_idx) })
@@ -206,7 +219,7 @@ impl<T, A: Allocator> DoubleEndedIterator for Drain<'_, T, A> {
206219
return None;
207220
}
208221
self.remaining -= 1;
209-
let wrapped_idx = unsafe { self.deque.as_ref().wrap_idx(self.idx + self.remaining) };
222+
let wrapped_idx = unsafe { self.deque.as_ref().to_physical_idx(self.idx + self.remaining) };
210223
Some(unsafe { self.deque.as_mut().buffer_read(wrapped_idx) })
211224
}
212225
}

‎library/alloc/src/collections/vec_deque/iter.rs

+14-16
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,6 @@ impl<T> Clone for Iter<'_, T> {
3939
impl<'a, T> Iterator for Iter<'a, T> {
4040
type Item = &'a T;
4141

42-
fn advance_by(&mut self, n: usize) -> Result<(), usize> {
43-
let m = match self.i1.advance_by(n) {
44-
Ok(_) => return Ok(()),
45-
Err(m) => m,
46-
};
47-
mem::swap(&mut self.i1, &mut self.i2);
48-
self.i1.advance_by(n - m).map_err(|o| o + m)
49-
}
50-
5142
#[inline]
5243
fn next(&mut self) -> Option<&'a T> {
5344
match self.i1.next() {
@@ -64,6 +55,15 @@ impl<'a, T> Iterator for Iter<'a, T> {
6455
}
6556
}
6657

58+
fn advance_by(&mut self, n: usize) -> Result<(), usize> {
59+
let m = match self.i1.advance_by(n) {
60+
Ok(_) => return Ok(()),
61+
Err(m) => m,
62+
};
63+
mem::swap(&mut self.i1, &mut self.i2);
64+
self.i1.advance_by(n - m).map_err(|o| o + m)
65+
}
66+
6767
#[inline]
6868
fn size_hint(&self) -> (usize, Option<usize>) {
6969
let len = self.len();
@@ -75,17 +75,16 @@ impl<'a, T> Iterator for Iter<'a, T> {
7575
F: FnMut(Acc, Self::Item) -> Acc,
7676
{
7777
let accum = self.i1.fold(accum, &mut f);
78-
self.i2.fold(accum, f)
78+
self.i2.fold(accum, &mut f)
7979
}
8080

8181
fn try_fold<B, F, R>(&mut self, init: B, mut f: F) -> R
8282
where
83-
Self: Sized,
8483
F: FnMut(B, Self::Item) -> R,
8584
R: Try<Output = B>,
8685
{
8786
let acc = self.i1.try_fold(init, &mut f)?;
88-
self.i2.try_fold(acc, f)
87+
self.i2.try_fold(acc, &mut f)
8988
}
9089

9190
#[inline]
@@ -117,7 +116,7 @@ impl<'a, T> DoubleEndedIterator for Iter<'a, T> {
117116
None => {
118117
// most of the time, the iterator will either always
119118
// call next(), or always call next_back(). By swapping
120-
// the iterators once the first one is empty, we ensure
119+
// the iterators once the second one is empty, we ensure
121120
// that the first branch is taken as often as possible,
122121
// without sacrificing correctness, as i2 is empty anyways
123122
mem::swap(&mut self.i1, &mut self.i2);
@@ -141,17 +140,16 @@ impl<'a, T> DoubleEndedIterator for Iter<'a, T> {
141140
F: FnMut(Acc, Self::Item) -> Acc,
142141
{
143142
let accum = self.i2.rfold(accum, &mut f);
144-
self.i1.rfold(accum, f)
143+
self.i1.rfold(accum, &mut f)
145144
}
146145

147146
fn try_rfold<B, F, R>(&mut self, init: B, mut f: F) -> R
148147
where
149-
Self: Sized,
150148
F: FnMut(B, Self::Item) -> R,
151149
R: Try<Output = B>,
152150
{
153151
let acc = self.i2.try_rfold(init, &mut f)?;
154-
self.i1.try_rfold(acc, f)
152+
self.i1.try_rfold(acc, &mut f)
155153
}
156154
}
157155

‎library/alloc/src/collections/vec_deque/iter_mut.rs

+4-6
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,16 @@ impl<'a, T> Iterator for IterMut<'a, T> {
6767
F: FnMut(Acc, Self::Item) -> Acc,
6868
{
6969
let accum = self.i1.fold(accum, &mut f);
70-
self.i2.fold(accum, f)
70+
self.i2.fold(accum, &mut f)
7171
}
7272

7373
fn try_fold<B, F, R>(&mut self, init: B, mut f: F) -> R
7474
where
75-
Self: Sized,
7675
F: FnMut(B, Self::Item) -> R,
7776
R: Try<Output = B>,
7877
{
7978
let acc = self.i1.try_fold(init, &mut f)?;
80-
self.i2.try_fold(acc, f)
79+
self.i2.try_fold(acc, &mut f)
8180
}
8281

8382
#[inline]
@@ -133,17 +132,16 @@ impl<'a, T> DoubleEndedIterator for IterMut<'a, T> {
133132
F: FnMut(Acc, Self::Item) -> Acc,
134133
{
135134
let accum = self.i2.rfold(accum, &mut f);
136-
self.i1.rfold(accum, f)
135+
self.i1.rfold(accum, &mut f)
137136
}
138137

139138
fn try_rfold<B, F, R>(&mut self, init: B, mut f: F) -> R
140139
where
141-
Self: Sized,
142140
F: FnMut(B, Self::Item) -> R,
143141
R: Try<Output = B>,
144142
{
145143
let acc = self.i2.try_rfold(init, &mut f)?;
146-
self.i1.try_rfold(acc, f)
144+
self.i1.try_rfold(acc, &mut f)
147145
}
148146
}
149147

0 commit comments

Comments
 (0)