Implement 1581 (FusedIterator) #35656
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
Please wait for the Travis run to complete before handing off to bors. I haven't done a full bootstrap build yet. |
|
Thanks for the PR @Stebalien! I think it's fine to start conservatively so we don't necessarily need to tag all the iterators today. I wonder though if this could use a similar trick to |
You can but it makes the implementation a lot messier. See #32999 (comment). However, after thinking about it a bit, it does make the documentation simpler (we can claim "no overhead" without qualifying what we mean by overhead). I've added a commit that does this based on @apasel422's comment (#32999 (comment)).
I'm worried that |
|
|
||
| #[derive(Clone)] | ||
| struct RealFuse<I> { | ||
| done: bool, |
There was a problem hiding this comment.
My original suggestion of using
struct RealFuse<I> {
iter: Option<I>,
}can save even more space if the underlying iterator contains a NonZero that lets the Option space optimization kick in, but it does have the side effect of changing the time at which the underlying iterator's destructor is called.
There was a problem hiding this comment.
but it does have the side effect of changing the time at which the underlying iterator's destructor is called
That's why I switched to this version. The real problem is that the two specializations of Fuse would behave very differently in an unexpected manner.
For example, say you have an iterator iterator holds a lock but you always use it under a Fuse wrapper. Now, someone implements FusedIterator on the underlying iterator. Suddenly, the inner iterator wouldn't be eagerly dropped and your code could deadlock.
|
I've addressed your feedback. I can fixup the commits into a logical set of if you want but left them "historically accurate" for review purposes. |
alexcrichton
added
the
T-libs-api
| } | ||
|
|
||
| fn spec_next(&mut self) -> Option<<Self::Iter as Iterator>::Item> | ||
| where Self::Iter: Iterator { |
There was a problem hiding this comment.
Stylistically we tend to put the { here on the next line like:
fn foo()
where // ...
{
// ...
}
|
Looks like the compile failure on travis may be legitimate? |
|
@alexcrichton done (I don't know if you're notified of commits...). |
|
Ok, look good to me! Can you add a few tests just as a smoke test as well that Other than that with a squash this looks good to me. |
| #[must_use = "iterator adaptors are lazy and do nothing unless consumed"] | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| pub struct Fuse<I> { | ||
| iter: I, | ||
| done: bool | ||
| iter: <I as SpecFuse>::T, |
There was a problem hiding this comment.
I suspect that this is going to change Fuse<I> from covariant in I to invariant in I: https://is.gd/btpbYx
There was a problem hiding this comment.
That's not good. Is there any way to fix that?
There was a problem hiding this comment.
Ah. I see how to fix this... Actually, it might make the code cleaner.
There was a problem hiding this comment.
I'll be curious to see your approach, as I earlier reported another instance of this issue here: #30642 (comment)
There was a problem hiding this comment.
Oh dear (#35727).
Well, I thought that I could do the same thing as the Zip specialization and put the flag in a separate static data field (keeping the iterator in the main struct) but apparently that doesn't appease rust:
struct Fuse<I> {
iter: I,
fused: <I as FuseImpl>::Data,
}There must be some way to do this...
|
☔ The latest upstream changes (presumably #35747) made this pull request unmergeable. Please resolve the merge conflicts. |
This trait can be used to avoid the overhead of a fuse wrapper when an iterator is already well-behaved. Conforming to: RFC 1581 Closes: rust-lang#35602
|
So, I've thrown away the associated type specialization (breaking change), squashed, and rebased. |
|
Note. I've yet to add sanity tests. I'll do that soon (hopefully). |
|
Hm I would personally be very wary of making the methods of the iterator trait |
|
This makes the methods of the implementation of |
|
Oh whoops! Sorry about that I did indeed misread. @aturon just to confirm, but that's right in that no one else external to the standard library can specialize further here? |
|
Discussed in @rust-lang/libs triage today and the conclusion was that this is good to go. Thanks @Stebalien! @bors: r+ |
|
📌 Commit de91872 has been approved by |
|
@alexcrichton (FYI, I still haven't added the sanity tests yet but thesis is due on 8 days so I'd rather not work on rust-proper until after that deadline). |
Implement 1581 (FusedIterator) * [ ] Implement on patterns. See #27721 (comment). * [ ] Handle OS Iterators. A bunch of iterators (`Args`, `Env`, etc.) in libstd wrap platform specific iterators. The current ones all appear to be well-behaved but can we assume that future ones will be? * [ ] Does someone want to audit this? On first glance, all of the iterators on which I implemented `FusedIterator` appear to be well-behaved but there are a *lot* of them so a second pair of eyes would be nice. * I haven't touched rustc internal iterators (or the internal rand) because rustc doesn't actually call `fuse()`. * `FusedIterator` can't be implemented on `std::io::{Bytes, Chars}`. Closes: #35602 (Tracking Issue) Implements: rust-lang/rfcs#1581
Add a test to ensure Fuse stays covariant When rust-lang#70502 attempted to specialize the data types in `Fuse`, one of the problems we found was that it broke variance. This was also realized when `Fuse` was first added, rust-lang#35656 (diff), but now this PR adds a test so we don't forget again.
Args,Env, etc.) in libstd wrap platform specific iterators. The current ones all appear to be well-behaved but can we assume that future ones will be?FusedIteratorappear to be well-behaved but there are a lot of them so a second pair of eyes would be nice.fuse().FusedIteratorcan't be implemented onstd::io::{Bytes, Chars}.Closes: #35602 (Tracking Issue)
Implements: rust-lang/rfcs#1581