Implement O(1) slice::Iter{,Mut} methods. #24701
Conversation
Instead of using the O(n) defaults, define O(1) shortcuts.
|
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @aturon |
|
|
||
| #[inline] | ||
| fn last(self) -> Option<$elem> { | ||
| // We could just call next_back but this avoids the memory write. |
There was a problem hiding this comment.
I feel like next_back is better: it seems like the write will often be eliminated anyway, and even if it isn't, it's cheap: i.e. the unsafe code isn't worth it.
1. Use next_back for last. 2. Use slices for computing nth. It might be possible to use the same code for both the mut/const case but I don't know how that will play with compiler optimizations.
| // Helper function for IterMut::nth | ||
| fn iter_nth(&mut self, n: usize) -> Option<&'a mut T> { | ||
| match make_mut_slice!(T => &'a mut [T]: self.ptr, self.end).get_mut(n) { | ||
| Some(elem_ref) => if mem::size_of::<T>() == 0 { |
There was a problem hiding this comment.
The logic here (especially with 0-sized types) is unfortunately duplicated with the next functions, would it be possible to deduplicate these conditionals?
There was a problem hiding this comment.
I ended up using a macro to work around the mut/const problem. I could have used custom traits but that would have been a lot of extra code for little functionality... Something like (untested):
trait Reference<T> {
type Pointer;
unsafe fn as_ptr(&self) -> Self::Pointer {
self as Self::Pointer
}
}
trait Pointer<T> {
type Reference;
unsafe fn as_ref(&self) -> Self::Reference {
transmute(self)
}
unsafe fn as_slice_ref(&self) -> Self::Reference {
if mem::size_of::<T>() == 0 {
&mut *(1 as *mut _)
} else {
self.as_ref()
}
}
unsafe fn slice_offset(n: isize) -> Self;
}
impl<T> Reference<T> for &T {
type Pointer = *const T;
}
impl<T> Reference<T> for &mut T {
type Pointer = *mut T;
}
impl<T> Pointer<T> for *const T {
type Reference = &T;
unsafe fn slice_offset(n: isize) -> Self {
if mem::size_of::<T>() == 0 {
transmute(self as usize + n)
} else {
self.offset(n)
}
}
}
impl<T> Pointer<T> for *mut T {
type Reference = &mut T;
unsafe fn slice_offset(n: isize) -> Self {
if mem::size_of::<T>() == 0 {
transmute(self as usize + n)
} else {
self.offset(n)
}
}
}
|
Why do slice iterators return |
| match self.as_slice().get(n) { | ||
| Some(elem_ref) => unsafe { | ||
| self.ptr = slice_offset!(elem_ref as *const _, 1); | ||
| Some(slice_ref!(elem_ref)) |
There was a problem hiding this comment.
I'm not 100% clear on why the slice_ref macro is needed here -- doesn't elem_ref already have the correct type and semantics?
There was a problem hiding this comment.
Hence my question:
Why do slice iterators return &mut *(1 as *mut) (references to 0x1) instead of the actual pointer into the slice (self.ptr)?
elem_ref is the actual pointer but slice_ref! will convert it to &mut *(1 as *mut _) if the type is zero sized.
There was a problem hiding this comment.
This may be a relic of an era since long past, I think elem_ref should be usable as-is today?
There was a problem hiding this comment.
I wonder if making this an actual pointer will have a performance impact. As-is, iterators over zero-sized types always yield constants.
There was a problem hiding this comment.
Hm that's true, I think it's fine to investigate this on the side though as the rest of this PR should be good to go, thanks @Stebalien!
|
Sorry for the delay reviewing this (please feel free to ping me on IRC, my inbox was recently a bit crazy and I was missing some things). This PR looks good to me -- I left you a minor question, but otherwise I'm ready to send it to bors. Thanks for doing this! |
Instead of using the O(n) defaults, define O(1) shortcuts. I also copied (and slightly modified) the relevant tests from the iter tests into the slice tests just in case someone comes along and changes them in the future. Partially implements #24214.
Instead of using the O(n) defaults, define O(1) shortcuts. I also copied (and slightly modified) the relevant tests from the iter tests into the slice tests just in case someone comes along and changes them in the future.
Partially implements #24214.