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

Implement specialized nth_back() for Zip #68199

Closed
wants to merge 1 commit into from
Closed

Implement specialized nth_back() for Zip #68199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 60 additions & 0 deletions src/libcore/iter/adapters/zip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,22 @@ impl<A: Iterator, B: Iterator> Zip<A, B> {
}
}

impl<A: DoubleEndedIterator, B: DoubleEndedIterator> Zip<A, B>
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator,
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
{
fn super_nth_back(&mut self, mut n: usize) -> Option<(A::Item, B::Item)> {
while let Some(x) = DoubleEndedIterator::next_back(self) {
if n == 0 {
return Some(x);
}
n -= 1;
}
None
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<A, B> Iterator for Zip<A, B>
where
Expand Down Expand Up @@ -70,6 +86,11 @@ where
fn next_back(&mut self) -> Option<(A::Item, B::Item)> {
ZipImpl::next_back(self)
}

#[inline]
fn nth_back(&mut self, n: usize) -> Option<Self::Item> {
ZipImpl::nth_back(self, n)
}
}

// Zip specialization trait
Expand All @@ -84,6 +105,10 @@ trait ZipImpl<A, B> {
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator;
fn nth_back(&mut self, n: usize) -> Option<Self::Item>
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator;
}

// General Zip impl
Expand Down Expand Up @@ -142,6 +167,15 @@ where
}
}

#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this specialization is unsound, though it's not a pre-existing problem, and not specific to this PR. See my comment here for a bit more context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, so I think we should stop the work for #54054 until the problem(#67194) is resolved, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe -- there are specific patterns that are sound -- but you have to specialize not based on traits but based on concrete types. e.g., specializing for vec::Iter or something would be fine. But maybe there are so many types here that this is not practical?

Copy link
Member Author

Choose a reason for hiding this comment

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

but you have to specialize not based on traits but based on concrete types. e.g., specializing for vec::Iter or something would be fine.

Yeah, it'd be an alternative. But unfortunately, I have no time to find alternative implementation. If I find some time, I'll re-open or submit another PR. Thanks for the review, Niko!

default fn nth_back(&mut self, n: usize) -> Option<Self::Item>
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator,
{
self.super_nth_back(n)
}

#[inline]
default fn size_hint(&self) -> (usize, Option<usize>) {
let (a_lower, a_upper) = self.a.size_hint();
Expand Down Expand Up @@ -248,6 +282,32 @@ where
None
}
}

#[inline]
fn nth_back(&mut self, n: usize) -> Option<Self::Item>
where
A: DoubleEndedIterator + ExactSizeIterator,
B: DoubleEndedIterator + ExactSizeIterator,
{
let delta = cmp::min(n, self.index);
let end = self.index - delta;
while end < self.index {
let i = self.index;
self.index -= 1;
if A::may_have_side_effect() {
unsafe {
self.a.get_unchecked(i);
}
}
if B::may_have_side_effect() {
unsafe {
self.b.get_unchecked(i);
}
}
}

self.super_nth_back(n - delta)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
37 changes: 37 additions & 0 deletions src/libcore/tests/iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,22 @@ fn test_zip_nth() {
assert_eq!(it.nth(3), None);
}

#[test]
fn test_zip_nth_back() {
let xs = [0, 1, 2, 4, 5];
let ys = [10, 11, 12];
let mut it = xs.iter().zip(&ys);
assert_eq!(it.nth_back(0), Some((&2, &12)));
assert_eq!(it.nth_back(1), Some((&0, &10)));
assert_eq!(it.nth_back(0), None);

let mut it = xs.iter().zip(&ys);
assert_eq!(it.nth_back(3), None);

let mut it = ys.iter().zip(&xs);
assert_eq!(it.nth_back(3), None);
}

#[test]
fn test_zip_nth_side_effects() {
let mut a = Vec::new();
Expand All @@ -291,6 +307,27 @@ fn test_zip_nth_side_effects() {
assert_eq!(b, vec![200, 300, 400, 500, 600]);
}

#[test]
fn test_zip_nth_back_side_effects() {
let mut a = Vec::new();
let mut b = Vec::new();
let value = [1, 2, 3, 4, 5, 6]
.iter()
.cloned()
.map(|n| {
a.push(n);
n * 10
})
.zip([2, 3, 4, 5, 6, 7, 8].iter().cloned().map(|n| {
b.push(n * 100);
n * 1000
}))
.nth_back(3);
assert_eq!(value, Some((30, 4000)));
assert_eq!(a, vec![6, 6, 5, 5, 4, 4, 3]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct and appears to be a bug that exists in the current next_back implementation. This should only contain each number once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, so the current nth_back implementation is also wrong?
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=ad07ad6a7077e2ba38eb232eed5e15bc

Copy link
Contributor

Choose a reason for hiding this comment

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

It's calling next_back, so yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed it as #68536 (I think this PR encounters another roadblock that Niko pointed out, so left a new issue).

assert_eq!(b, vec![800, 700, 700, 600, 600, 500, 500, 400]);
}

#[test]
fn test_iterator_step_by() {
// Identity
Expand Down