Skip to content

Add new iterator function .chunk() #8376

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

Closed
wants to merge 1 commit into from

Conversation

lilyball
Copy link
Contributor

@lilyball lilyball commented Aug 7, 2013

.chunk() takes a uint and yields owned vectors of that length.

r? @thestinger

@lilyball
Copy link
Contributor Author

lilyball commented Aug 7, 2013

@cmr: Based on @thestinger's comments on my PR regarding Skip (#8375), I'm modifying this one as well (since it also tried to peg Some(uint::max_value) in the upper bound).

.chunk() takes a uint and yields owned vectors of that length.
@lilyball
Copy link
Contributor Author

lilyball commented Aug 7, 2013

r? @cmr

@lilyball
Copy link
Contributor Author

lilyball commented Aug 7, 2013

@cmr Regarding your comment about a test for n >= num elts, that's actually covered by the existing test. In the first assert, the final chunk is n == num elts and in the second assert, the final chunk is n < num elts.

@emberian
Copy link
Member

emberian commented Aug 7, 2013

@kballard I remain unconvinced

@thestinger
Copy link
Contributor

I don't think this is how we should implement chunk - it's going to be far too inefficient. It shouldn't require an allocation for every iteration - this kind of thing doesn't belong as an adaptor in std::iterator in my opinion unless we can do it efficiently and without special-casing container types like ~[T].

@thestinger
Copy link
Contributor

For example, this could yield an iterator of iterators.

@lilyball
Copy link
Contributor Author

lilyball commented Aug 7, 2013

@thestinger An iterator of iterators is actually a good idea. I originally wanted to yield &[A] but there's no way to do that safely. engla on IRC suggested that returning a ~[A] instead of &[A] is actually ok because you can't move out of an &[A] anyway. But I think an iterator of iterators is even better, because you can just call .to_owned_vec() to recreate the ~[A] approach.

I'm going to close out this PR for the time being, I'll re-submit when I've implemented @thestinger's idea (not sure when though, as I'm going on vacation).

@lilyball lilyball closed this Aug 7, 2013
@thestinger
Copy link
Contributor

I'm not 100% sure it will work, but I think it will.

@lilyball
Copy link
Contributor Author

lilyball commented Aug 7, 2013

@thestinger Upon thinking about it, I think this may hit the same issue that trying to yield &[A] does, which is you can't put a lifetime parameter on the &mut self arg to next(), so your resulting value can't have its lifetime tied to the ChunkIter. And because of that, the yielded iterators cannot keep a &mut ref to the underlying iterator (at least, not safely).

What it could do is buffer n elements into a ~[A] and yield the consume_iter() from that, but that's rather pointless compared to just yielding the ~[A] directly.

@nikomatsakis
Copy link
Contributor

I am not sure if an iterator-that-yields-iterators can be done using the iterator trait and type system as it is today. As @kballard points out, it'd have to "give up" the underlying iterator temporarily, and the lifetime for which it would have to "give up" the underlying iterator is not known until you call next(), so it can't be encoded into the type you are iterating over.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 10, 2022
[`chars_next_cmp`] Fix unescaped suggestion

closes rust-lang#8373

changelog: [`chars_next_cmp`] Fix unescaped suggestion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants