Skip to content

Commit 5b24e12

Browse files
borsbors
committed
Auto merge of rust-lang#111395 - scottmcm:slice-iter-zst-experiment, r=the8472
Simplify the implementation of iterators over slices of ZSTs Currently, slice iterators over ZSTs store `end = start.wrapping_byte_add(len)`. That's slightly convenient for `is_empty`, but kinda annoying for pretty much everything else -- see bugs like rust-lang#42789, for example. This PR instead changes it to just `end = ptr::invalid(len)` instead. That's easier to think about (IMHO, at least) as well as easier to represent. `next` is still to big to get inlined into the mir-opt/pre-codegen/ tests, but if I bump the inline threshold to force it to show the whole thing, this implementation is also less MIR: ``` > git diff --numstat 241 370 tests/mir-opt/pre-codegen/slice_iter.forward_loop.PreCodegen.after.mir 255 329 tests/mir-opt/pre-codegen/slice_iter.reverse_loop.PreCodegen.after.mir 184 216 tests/mir-opt/pre-codegen/slice_iter.slice_iter_mut_next_back.PreCodegen.after.mir 182 254 tests/mir-opt/pre-codegen/slice_iter.slice_iter_next.PreCodegen.after.mir ``` (That's ≈70 lines less for `Iter::next`, for example.) r? `@ghost` ~~Built atop rust-lang#111282, so draft until that lands.~~
2 parents 2a8221d + 15aa7fa commit 5b24e12

File tree

3 files changed

+51
-35
lines changed

3 files changed

+51
-35
lines changed

‎library/core/src/ptr/non_null.rs

+13
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,19 @@ impl<T: ?Sized> NonNull<T> {
449449
// SAFETY: `self` is a `NonNull` pointer which is necessarily non-null
450450
unsafe { NonNull::new_unchecked(self.as_ptr() as *mut U) }
451451
}
452+
453+
/// See [`pointer::add`] for semantics and safety requirements.
454+
#[inline]
455+
pub(crate) const unsafe fn add(self, delta: usize) -> Self
456+
where
457+
T: Sized,
458+
{
459+
// SAFETY: We require that the delta stays in-bounds of the object, and
460+
// thus it cannot become null, as that would require wrapping the
461+
// address space, which no legal objects are allowed to do.
462+
// And the caller promised the `delta` is sound to add.
463+
unsafe { NonNull { pointer: self.pointer.add(delta) } }
464+
}
452465
}
453466

454467
impl<T> NonNull<[T]> {

‎library/core/src/slice/iter.rs

+5-11
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use crate::iter::{
1313
use crate::marker::{PhantomData, Send, Sized, Sync};
1414
use crate::mem::{self, SizedTypeProperties};
1515
use crate::num::NonZeroUsize;
16-
use crate::ptr::NonNull;
16+
use crate::ptr::{invalid, invalid_mut, NonNull};
1717

1818
use super::{from_raw_parts, from_raw_parts_mut};
1919

@@ -67,9 +67,7 @@ pub struct Iter<'a, T: 'a> {
6767
ptr: NonNull<T>,
6868
/// For non-ZSTs, the non-null pointer to the past-the-end element.
6969
///
70-
/// For ZSTs, this is `ptr.wrapping_byte_add(len)`.
71-
///
72-
/// For all types, `ptr == end` tests whether the iterator is empty.
70+
/// For ZSTs, this is `ptr::invalid(len)`.
7371
end: *const T,
7472
_marker: PhantomData<&'a T>,
7573
}
@@ -94,8 +92,7 @@ impl<'a, T> Iter<'a, T> {
9492
unsafe {
9593
assume(!ptr.is_null());
9694

97-
let end =
98-
if T::IS_ZST { ptr.wrapping_byte_add(slice.len()) } else { ptr.add(slice.len()) };
95+
let end = if T::IS_ZST { invalid(slice.len()) } else { ptr.add(slice.len()) };
9996

10097
Self { ptr: NonNull::new_unchecked(ptr as *mut T), end, _marker: PhantomData }
10198
}
@@ -193,9 +190,7 @@ pub struct IterMut<'a, T: 'a> {
193190
ptr: NonNull<T>,
194191
/// For non-ZSTs, the non-null pointer to the past-the-end element.
195192
///
196-
/// For ZSTs, this is `ptr.wrapping_byte_add(len)`.
197-
///
198-
/// For all types, `ptr == end` tests whether the iterator is empty.
193+
/// For ZSTs, this is `ptr::invalid_mut(len)`.
199194
end: *mut T,
200195
_marker: PhantomData<&'a mut T>,
201196
}
@@ -235,8 +230,7 @@ impl<'a, T> IterMut<'a, T> {
235230
unsafe {
236231
assume(!ptr.is_null());
237232

238-
let end =
239-
if T::IS_ZST { ptr.wrapping_byte_add(slice.len()) } else { ptr.add(slice.len()) };
233+
let end = if T::IS_ZST { invalid_mut(slice.len()) } else { ptr.add(slice.len()) };
240234

241235
Self { ptr: NonNull::new_unchecked(ptr), end, _marker: PhantomData }
242236
}

‎library/core/src/slice/iter/macros.rs

+33-24
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,44 @@
11
//! Macros used by iterators of slice.
22
3+
// Shrinks the iterator when T is a ZST, setting the length to `new_len`.
4+
// `new_len` must not exceed `self.len()`.
5+
macro_rules! zst_set_len {
6+
($self: ident, $new_len: expr) => {{
7+
#![allow(unused_unsafe)] // we're sometimes used within an unsafe block
8+
9+
// SAFETY: same as `invalid(_mut)`, but the macro doesn't know
10+
// which versions of that function to call, so open-code it.
11+
$self.end = unsafe { mem::transmute::<usize, _>($new_len) };
12+
}};
13+
}
14+
15+
// Shrinks the iterator when T is a ZST, reducing the length by `n`.
16+
// `n` must not exceed `self.len()`.
17+
macro_rules! zst_shrink {
18+
($self: ident, $n: ident) => {
19+
let new_len = $self.end.addr() - $n;
20+
zst_set_len!($self, new_len);
21+
};
22+
}
23+
324
// Inlining is_empty and len makes a huge performance difference
425
macro_rules! is_empty {
5-
// The way we encode the length of a ZST iterator, this works both for ZST
6-
// and non-ZST.
726
($self: ident) => {
8-
$self.ptr.as_ptr() as *const T == $self.end
27+
if T::IS_ZST { $self.end.addr() == 0 } else { $self.ptr.as_ptr() as *const _ == $self.end }
928
};
1029
}
1130

1231
macro_rules! len {
1332
($self: ident) => {{
1433
#![allow(unused_unsafe)] // we're sometimes used within an unsafe block
1534

16-
let start = $self.ptr;
1735
if T::IS_ZST {
18-
// This _cannot_ use `ptr_sub` because we depend on wrapping
19-
// to represent the length of long ZST slice iterators.
20-
$self.end.addr().wrapping_sub(start.as_ptr().addr())
36+
$self.end.addr()
2137
} else {
2238
// To get rid of some bounds checks (see `position`), we use ptr_sub instead of
2339
// offset_from (Tested by `codegen/slice-position-bounds-check`.)
2440
// SAFETY: by the type invariant pointers are aligned and `start <= end`
25-
unsafe { $self.end.sub_ptr(start.as_ptr()) }
41+
unsafe { $self.end.sub_ptr($self.ptr.as_ptr()) }
2642
}
2743
}};
2844
}
@@ -50,14 +66,6 @@ macro_rules! iterator {
5066
($self: ident) => {& $( $mut_ )? *$self.pre_dec_end(1)}
5167
}
5268

53-
// Shrinks the iterator when T is a ZST, by moving the end of the iterator
54-
// backwards by `n`. `n` must not exceed `self.len()`.
55-
macro_rules! zst_shrink {
56-
($self: ident, $n: ident) => {
57-
$self.end = $self.end.wrapping_byte_sub($n);
58-
}
59-
}
60-
6169
impl<'a, T> $name<'a, T> {
6270
// Helper function for creating a slice from the iterator.
6371
#[inline(always)]
@@ -73,16 +81,15 @@ macro_rules! iterator {
7381
// Unsafe because the offset must not exceed `self.len()`.
7482
#[inline(always)]
7583
unsafe fn post_inc_start(&mut self, offset: usize) -> * $raw_mut T {
84+
let old = self.ptr;
7685
if T::IS_ZST {
7786
zst_shrink!(self, offset);
78-
self.ptr.as_ptr()
7987
} else {
80-
let old = self.ptr.as_ptr();
8188
// SAFETY: the caller guarantees that `offset` doesn't exceed `self.len()`,
8289
// so this new pointer is inside `self` and thus guaranteed to be non-null.
83-
self.ptr = unsafe { NonNull::new_unchecked(self.ptr.as_ptr().add(offset)) };
84-
old
90+
self.ptr = unsafe { self.ptr.add(offset) };
8591
}
92+
old.as_ptr()
8693
}
8794

8895
// Helper function for moving the end of the iterator backwards by `offset` elements,
@@ -155,9 +162,7 @@ macro_rules! iterator {
155162
if n >= len!(self) {
156163
// This iterator is now empty.
157164
if T::IS_ZST {
158-
// We have to do it this way as `ptr` may never be 0, but `end`
159-
// could be (due to wrapping).
160-
self.end = self.ptr.as_ptr();
165+
zst_set_len!(self, 0);
161166
} else {
162167
// SAFETY: end can't be 0 if T isn't ZST because ptr isn't 0 and end >= ptr
163168
unsafe {
@@ -356,7 +361,11 @@ macro_rules! iterator {
356361
fn nth_back(&mut self, n: usize) -> Option<$elem> {
357362
if n >= len!(self) {
358363
// This iterator is now empty.
359-
self.end = self.ptr.as_ptr();
364+
if T::IS_ZST {
365+
zst_set_len!(self, 0);
366+
} else {
367+
self.end = self.ptr.as_ptr();
368+
}
360369
return None;
361370
}
362371
// SAFETY: We are in bounds. `pre_dec_end` does the right thing even for ZSTs.

0 commit comments

Comments
 (0)