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.

Sign up for GitHub

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

Jump to bottom

Carefully remove bounds checks from some chunk iterator functions #86988

Merged
merged 5 commits into from
Feb 1, 2022
Merged

Carefully remove bounds checks from some chunk iterator functions #86988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
83aa6d4
Carefully remove bounds checks from some chunk iterators
thomcc Jul 8, 2021
e81fefa
Address some issues in chunk iterator safety comments
thomcc Oct 30, 2021
b543816
Reference Chunks::next_back in more of the chunk iterators safety com…
thomcc Oct 30, 2021
b8abd55
Pull `self.v.len()` out in `RChunks::next` as suggested in review com…
thomcc Oct 30, 2021
9c62455
Improve test coverage of {Chunks,RChunks,RChunksMut}::{next,next_back}
thomcc Feb 1, 2022
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
42 changes: 35 additions & 7 deletions library/core/src/slice/iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,7 +1474,21 @@ impl<'a, T> DoubleEndedIterator for Chunks<'a, T> {
} else {
let remainder = self.v.len() % self.chunk_size;
let chunksz = if remainder != 0 { remainder } else { self.chunk_size };
let (fst, snd) = self.v.split_at(self.v.len() - chunksz);
// SAFETY: split_at_unchecked requires the argument be less than or
// equal to the length. This is guaranteed, but subtle: `chunksz`
// will always either be `self.v.len() % self.chunk_size`, which
// will always evaluate to strictly less than `self.v.len()` (or
// panic, in the case that `self.chunk_size` is zero), or it can be
// `self.chunk_size`, in the case that the length is exactly
// divisible by the chunk size.
//
// While it seems like using `self.chunk_size` in this case could
// lead to a value greater than `self.v.len()`, it cannot: if
// `self.chunk_size` were greater than `self.v.len()`, then
// `self.v.len() % self.chunk_size` would return nonzero (note that
// in this branch of the `if`, we already know that `self.v` is
// non-empty).
let (fst, snd) = unsafe { self.v.split_at_unchecked(self.v.len() - chunksz) };
self.v = fst;
Some(snd)
}
Expand Down Expand Up @@ -1639,7 +1653,8 @@ impl<'a, T> DoubleEndedIterator for ChunksMut<'a, T> {
let sz = if remainder != 0 { remainder } else { self.chunk_size };
let tmp = mem::replace(&mut self.v, &mut []);
let tmp_len = tmp.len();
let (head, tail) = tmp.split_at_mut(tmp_len - sz);
// SAFETY: Similar to `Chunks::next_back`
let (head, tail) = unsafe { tmp.split_at_mut_unchecked(tmp_len - sz) };
self.v = head;
Some(tail)
}
Expand Down Expand Up @@ -2408,8 +2423,14 @@ impl<'a, T> Iterator for RChunks<'a, T> {
if self.v.is_empty() {
None
} else {
let chunksz = cmp::min(self.v.len(), self.chunk_size);
let (fst, snd) = self.v.split_at(self.v.len() - chunksz);
let len = self.v.len();
let chunksz = cmp::min(len, self.chunk_size);
// SAFETY: split_at_unchecked just requires the argument be less
// than the length. This could only happen if the expression `len -
// chunksz` overflows. This could only happen if `chunksz > len`,
// which is impossible as we initialize it as the `min` of `len` and
// `self.chunk_size`.
let (fst, snd) = unsafe { self.v.split_at_unchecked(len - chunksz) };
self.v = fst;
Some(snd)
}
Expand Down Expand Up @@ -2483,7 +2504,8 @@ impl<'a, T> DoubleEndedIterator for RChunks<'a, T> {
} else {
let remainder = self.v.len() % self.chunk_size;
let chunksz = if remainder != 0 { remainder } else { self.chunk_size };
let (fst, snd) = self.v.split_at(chunksz);
// SAFETY: similar to Chunks::next_back
let (fst, snd) = unsafe { self.v.split_at_unchecked(chunksz) };
self.v = snd;
Some(fst)
}
Expand Down Expand Up @@ -2569,7 +2591,12 @@ impl<'a, T> Iterator for RChunksMut<'a, T> {
let sz = cmp::min(self.v.len(), self.chunk_size);
let tmp = mem::replace(&mut self.v, &mut []);
let tmp_len = tmp.len();
let (head, tail) = tmp.split_at_mut(tmp_len - sz);
// SAFETY: split_at_mut_unchecked just requires the argument be less
// than the length. This could only happen if the expression
// `tmp_len - sz` overflows. This could only happen if `sz >
// tmp_len`, which is impossible as we initialize it as the `min` of
// `self.v.len()` (e.g. `tmp_len`) and `self.chunk_size`.
let (head, tail) = unsafe { tmp.split_at_mut_unchecked(tmp_len - sz) };
self.v = head;
Some(tail)
}
Expand Down Expand Up @@ -2647,7 +2674,8 @@ impl<'a, T> DoubleEndedIterator for RChunksMut<'a, T> {
let remainder = self.v.len() % self.chunk_size;
let sz = if remainder != 0 { remainder } else { self.chunk_size };
let tmp = mem::replace(&mut self.v, &mut []);
let (head, tail) = tmp.split_at_mut(sz);
// SAFETY: Similar to `Chunks::next_back`
let (head, tail) = unsafe { tmp.split_at_mut_unchecked(sz) };
self.v = tail;
Some(head)
}
Expand Down
102 changes: 102 additions & 0 deletions library/core/tests/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,40 @@ fn test_chunks_nth() {
assert_eq!(c2.next(), None);
}

#[test]
fn test_chunks_next() {
thomcc marked this conversation as resolved.
Show resolved Hide resolved
let v = [0, 1, 2, 3, 4, 5];
let mut c = v.chunks(2);
assert_eq!(c.next().unwrap(), &[0, 1]);
assert_eq!(c.next().unwrap(), &[2, 3]);
assert_eq!(c.next().unwrap(), &[4, 5]);
assert_eq!(c.next(), None);

let v = [0, 1, 2, 3, 4, 5, 6, 7];
let mut c = v.chunks(3);
assert_eq!(c.next().unwrap(), &[0, 1, 2]);
assert_eq!(c.next().unwrap(), &[3, 4, 5]);
assert_eq!(c.next().unwrap(), &[6, 7]);
assert_eq!(c.next(), None);
}

#[test]
fn test_chunks_next_back() {
let v = [0, 1, 2, 3, 4, 5];
let mut c = v.chunks(2);
assert_eq!(c.next_back().unwrap(), &[4, 5]);
assert_eq!(c.next_back().unwrap(), &[2, 3]);
assert_eq!(c.next_back().unwrap(), &[0, 1]);
assert_eq!(c.next_back(), None);

let v = [0, 1, 2, 3, 4, 5, 6, 7];
let mut c = v.chunks(3);
assert_eq!(c.next_back().unwrap(), &[6, 7]);
assert_eq!(c.next_back().unwrap(), &[3, 4, 5]);
assert_eq!(c.next_back().unwrap(), &[0, 1, 2]);
assert_eq!(c.next_back(), None);
}

#[test]
fn test_chunks_nth_back() {
let v: &[i32] = &[0, 1, 2, 3, 4, 5];
Expand Down Expand Up @@ -807,6 +841,40 @@ fn test_rchunks_nth_back() {
assert_eq!(c2.next_back(), None);
}

#[test]
fn test_rchunks_next() {
let v = [0, 1, 2, 3, 4, 5];
let mut c = v.rchunks(2);
assert_eq!(c.next().unwrap(), &[4, 5]);
assert_eq!(c.next().unwrap(), &[2, 3]);
assert_eq!(c.next().unwrap(), &[0, 1]);
assert_eq!(c.next(), None);

let v = [0, 1, 2, 3, 4, 5, 6, 7];
let mut c = v.rchunks(3);
assert_eq!(c.next().unwrap(), &[5, 6, 7]);
assert_eq!(c.next().unwrap(), &[2, 3, 4]);
assert_eq!(c.next().unwrap(), &[0, 1]);
assert_eq!(c.next(), None);
}

#[test]
fn test_rchunks_next_back() {
let v = [0, 1, 2, 3, 4, 5];
let mut c = v.rchunks(2);
assert_eq!(c.next_back().unwrap(), &[0, 1]);
assert_eq!(c.next_back().unwrap(), &[2, 3]);
assert_eq!(c.next_back().unwrap(), &[4, 5]);
assert_eq!(c.next_back(), None);

let v = [0, 1, 2, 3, 4, 5, 6, 7];
let mut c = v.rchunks(3);
assert_eq!(c.next_back().unwrap(), &[0, 1]);
assert_eq!(c.next_back().unwrap(), &[2, 3, 4]);
assert_eq!(c.next_back().unwrap(), &[5, 6, 7]);
assert_eq!(c.next_back(), None);
}

#[test]
fn test_rchunks_last() {
let v: &[i32] = &[0, 1, 2, 3, 4, 5];
Expand Down Expand Up @@ -872,6 +940,40 @@ fn test_rchunks_mut_nth_back() {
assert_eq!(c2.next_back(), None);
}

#[test]
fn test_rchunks_mut_next() {
let mut v = [0, 1, 2, 3, 4, 5];
let mut c = v.rchunks_mut(2);
assert_eq!(c.next().unwrap(), &mut [4, 5]);
assert_eq!(c.next().unwrap(), &mut [2, 3]);
assert_eq!(c.next().unwrap(), &mut [0, 1]);
assert_eq!(c.next(), None);

let mut v = [0, 1, 2, 3, 4, 5, 6, 7];
let mut c = v.rchunks_mut(3);
assert_eq!(c.next().unwrap(), &mut [5, 6, 7]);
assert_eq!(c.next().unwrap(), &mut [2, 3, 4]);
assert_eq!(c.next().unwrap(), &mut [0, 1]);
assert_eq!(c.next(), None);
}

#[test]
fn test_rchunks_mut_next_back() {
let mut v = [0, 1, 2, 3, 4, 5];
let mut c = v.rchunks_mut(2);
assert_eq!(c.next_back().unwrap(), &mut [0, 1]);
assert_eq!(c.next_back().unwrap(), &mut [2, 3]);
assert_eq!(c.next_back().unwrap(), &mut [4, 5]);
assert_eq!(c.next_back(), None);

let mut v = [0, 1, 2, 3, 4, 5, 6, 7];
let mut c = v.rchunks_mut(3);
assert_eq!(c.next_back().unwrap(), &mut [0, 1]);
assert_eq!(c.next_back().unwrap(), &mut [2, 3, 4]);
assert_eq!(c.next_back().unwrap(), &mut [5, 6, 7]);
assert_eq!(c.next_back(), None);
}

#[test]
fn test_rchunks_mut_last() {
let v: &mut [i32] = &mut [0, 1, 2, 3, 4, 5];
Expand Down