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.

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

Merged
merged 3 commits into from
Apr 28, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
54 changes: 54 additions & 0 deletions src/libcore/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,60 @@ macro_rules! iterator {
let exact = diff / (if size == 0 {1} else {size});
(exact, Some(exact))
}

#[inline]
fn count(self) -> usize {
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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 :)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 abuse count for this today.

self.size_hint().0
}

#[inline]
fn nth(&mut self, n: usize) -> Option<$elem> {
// could be implemented with slices, but this avoids bounds checks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't: there's the pointer comparisons below. I think this would be better as self.as_slice().get(n).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

unsafe {
::intrinsics::assume(!self.ptr.is_null());
::intrinsics::assume(!self.end.is_null());
// There should be some way to use offset and optimize this to LEA but I don't
// know how to do that AND detect overflow...
let size = mem::size_of::<T>();
if size == 0 {
if let Some(new_ptr) = (self.ptr as usize).checked_add(n) {
if new_ptr < (self.end as usize) {
self.ptr = transmute(new_ptr + 1);
return Some(&mut *(1 as *mut _))
}
}
} else {
if let Some(new_ptr) = n.checked_mul(size).and_then(|offset| {
(self.ptr as usize).checked_add(offset)
}) {
if new_ptr < (self.end as usize) {
self.ptr = transmute(new_ptr + size);
return Some(transmute(new_ptr))
}
}
}
None
}
}

#[inline]
fn last(self) -> Option<$elem> {
// We could just call next_back but this avoids the memory write.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unsafe {
::intrinsics::assume(!self.ptr.is_null());
::intrinsics::assume(!self.end.is_null());
if self.end == self.ptr {
None
} else {
if mem::size_of::<T>() == 0 {
// Use a non-null pointer value
Some(&mut *(1 as *mut _))
} else {
Some(transmute(self.end.offset(-1)))
}
}
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
31 changes: 31 additions & 0 deletions src/libcoretest/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,34 @@ fn iterator_to_slice() {
test!([1u8,2,3]);
test!([(),(),()]);
}

#[test]
fn test_iterator_nth() {
let v: &[_] = &[0, 1, 2, 3, 4];
for i in 0..v.len() {
assert_eq!(v.iter().nth(i).unwrap(), &v[i]);
}
assert_eq!(v.iter().nth(v.len()), None);

let mut iter = v.iter();
assert_eq!(iter.nth(2).unwrap(), &v[2]);
assert_eq!(iter.nth(1).unwrap(), &v[4]);
}

#[test]
fn test_iterator_last() {
let v: &[_] = &[0, 1, 2, 3, 4];
assert_eq!(v.iter().last().unwrap(), &4);
assert_eq!(v[..1].iter().last().unwrap(), &0);
}

#[test]
fn test_iterator_count() {
let v: &[_] = &[0, 1, 2, 3, 4];
assert_eq!(v.iter().count(), 5);

let mut iter2 = v.iter();
iter2.next();
iter2.next();
assert_eq!(iter2.count(), 3);
}