-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 O(1) slice::Iter{,Mut} methods. #24701
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -625,6 +625,36 @@ impl<'a, T> IntoIterator for &'a mut [T] { | |
} | ||
} | ||
|
||
#[inline(always)] | ||
fn size_from_ptr<T>(_: *const T) -> usize { | ||
mem::size_of::<T>() | ||
} | ||
|
||
|
||
// Use macro to be generic over const/mut | ||
macro_rules! slice_offset { | ||
($ptr:expr, $by:expr) => {{ | ||
let ptr = $ptr; | ||
if size_from_ptr(ptr) == 0 { | ||
transmute(ptr as usize + $by) | ||
} else { | ||
ptr.offset($by) | ||
} | ||
}}; | ||
} | ||
|
||
macro_rules! slice_ref { | ||
($ptr:expr) => {{ | ||
let ptr = $ptr; | ||
if size_from_ptr(ptr) == 0 { | ||
// Use a non-null pointer value | ||
&mut *(1 as *mut _) | ||
} else { | ||
transmute(ptr) | ||
} | ||
}}; | ||
} | ||
|
||
// The shared definition of the `Iter` and `IterMut` iterators | ||
macro_rules! iterator { | ||
(struct $name:ident -> $ptr:ty, $elem:ty) => { | ||
|
@@ -641,20 +671,9 @@ macro_rules! iterator { | |
if self.ptr == self.end { | ||
None | ||
} else { | ||
if mem::size_of::<T>() == 0 { | ||
// purposefully don't use 'ptr.offset' because for | ||
// vectors with 0-size elements this would return the | ||
// same pointer. | ||
self.ptr = transmute(self.ptr as usize + 1); | ||
|
||
// Use a non-null pointer value | ||
Some(&mut *(1 as *mut _)) | ||
} else { | ||
let old = self.ptr; | ||
self.ptr = self.ptr.offset(1); | ||
|
||
Some(transmute(old)) | ||
} | ||
let old = self.ptr; | ||
self.ptr = slice_offset!(self.ptr, 1); | ||
Some(slice_ref!(old)) | ||
} | ||
} | ||
} | ||
|
@@ -666,6 +685,22 @@ macro_rules! iterator { | |
let exact = diff / (if size == 0 {1} else {size}); | ||
(exact, Some(exact)) | ||
} | ||
|
||
#[inline] | ||
fn count(self) -> usize { | ||
self.size_hint().0 | ||
} | ||
|
||
#[inline] | ||
fn nth(&mut self, n: usize) -> Option<$elem> { | ||
// Call helper method. Can't put the definition here because mut versus const. | ||
self.iter_nth(n) | ||
} | ||
|
||
#[inline] | ||
fn last(mut self) -> Option<$elem> { | ||
self.next_back() | ||
} | ||
} | ||
|
||
#[stable(feature = "rust1", since = "1.0.0")] | ||
|
@@ -679,17 +714,8 @@ macro_rules! iterator { | |
if self.end == self.ptr { | ||
None | ||
} else { | ||
if mem::size_of::<T>() == 0 { | ||
// See above for why 'ptr.offset' isn't used | ||
self.end = transmute(self.end as usize - 1); | ||
|
||
// Use a non-null pointer value | ||
Some(&mut *(1 as *mut _)) | ||
} else { | ||
self.end = self.end.offset(-1); | ||
|
||
Some(transmute(self.end)) | ||
} | ||
self.end = slice_offset!(self.end, -1); | ||
Some(slice_ref!(self.end)) | ||
} | ||
} | ||
} | ||
|
@@ -785,6 +811,20 @@ impl<'a, T> Iter<'a, T> { | |
pub fn as_slice(&self) -> &'a [T] { | ||
make_slice!(T => &'a [T]: self.ptr, self.end) | ||
} | ||
|
||
// Helper function for Iter::nth | ||
fn iter_nth(&mut self, n: usize) -> Option<&'a T> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be possible to share code between the iter_nth implementations but I don't know how safe transmuting/casting between const/mut pointers is... |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% clear on why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hence my question:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be a relic of an era since long past, I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
}, | ||
None => { | ||
self.ptr = self.end; | ||
None | ||
} | ||
} | ||
} | ||
} | ||
|
||
iterator!{struct Iter -> *const T, &'a T} | ||
|
@@ -914,6 +954,20 @@ impl<'a, T> IterMut<'a, T> { | |
pub fn into_slice(self) -> &'a mut [T] { | ||
make_mut_slice!(T => &'a mut [T]: self.ptr, self.end) | ||
} | ||
|
||
// 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) => unsafe { | ||
self.ptr = slice_offset!(elem_ref as *mut _, 1); | ||
Some(slice_ref!(elem_ref)) | ||
}, | ||
None => { | ||
self.ptr = self.end; | ||
None | ||
} | ||
} | ||
} | ||
} | ||
|
||
iterator!{struct IterMut -> *mut T, &'a mut T} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this does change the semantics of
count
from the default implementation which exhausts the iterator. Perhaps this could modify start to equal end so retain equivalent semantics?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er actually this applies to all of the functions below as well as in the semantics have changed with respect to the default implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, this consumes the iterator so it shouldn't matter. Is that not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that
size_hint
doesn't consume the iterator, so I don't think this consumes the iterator?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but the function itself (
count(self)
) does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dear that is a very good point! After rereading
nth
I see it's updating the pointers already, in which case you can just completely ignore me :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it changes the behavior if this was called on a
by_ref()
iterator. But these have odd behavior anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but then this count won't be called (it can't because it needs to consume the iterator by value). Instead the default (slow) Iterator count will be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole comment thread says to me that we need a specific
consume
method for just running through an iterator, because people abusecount
for this today.