Add Iterator::intersperse by camelid · Pull Request #79479 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
(rust-highfive has picked a reviewer for you, use r? to override)
This comment has been minimized.
This comment has been minimized.
Two cents: We could lift the I::Item: Clone bound by providing a method to the tune of Iterator::intersperse_with(seperator_gen: impl Fn() -> Self::Item). That way, we can do intersperse_with(|| Ok(1)), without requiring that Result<T, E> to be Clone, removing the Clone-bound from Intersperse. Only Iterator::intersperse has a bound then and becomes a shorthand which clones the given value over and over again internally.
Having thought about it again, I think we'd need a separate Iterator::intersperse_with() -> IntersperseWith, as Iterator::intersperse would have to need a return type akin to Intersperse<Self, impl FnMut -> Self::Item>, which we can't do in trait implementations as of now. Since intersperse.rs is small, the duplication wouldn't hurt. iter::Repeat and ::RepeatWith are precedent. Poke me for a PR if there is interest.
What would the opinion be on making it support fn intersperse<T: Into<Self::Item>>(self, separator: T)? (Please tell me if this would better go in the tracking issue/zulip)
The signature for this function is a good question. Since it only clones the separator, a reference to it would suffice. However, that'd make it a bit more complicated in usage, as it'd always be borrowing something, making it a bit tricky with the lifetime in some situations.
The most flexible signature would be that of intersperse_with (which I believe is added in itertools 0.10). If we also have that function, then intersperse itself is simply a more specific version of that function, just for one common case. So in that case, I'd be less worried about picking the perfect/most flexible signature for intersperse. With intersperse_with as the generic/flexible alternative, it makes sense for intersperse to just be the simplest version. It'd also avoid some breakage if it has the exact same signature as in itertools.
I don't want to derail this, as there has already been quite some discussion before in #75784 and related. To reiterate, I just think the Clone-bound is quite harsh for the general case and especially iterators that do not iterate over immutable references. An intersperse_with would lift this restriction and also address #79479 (comment), as the closure can do the required conversion without complicating the signature of intersperse_with (akin Into). Afaics, an implementation of intersperse can't reuse the machinery of intersperse_with, as we'd have to generate a closure which does the cloning and becomes part of the return type, which we can't do in a trait. However, there is precedent in Repeat and RepeatWith for exactly this kind of situation.
To the tune of
Iterator::intersperse_with<G>(self, separator_gen: G) -> IntersperseWith<Self, G> where Self:Sized, G: FnMut() -> Self::Item
Iterator::intersperse(self, separator: Self::Item) -> Intersperse<Self> where Self:Sized, Self::Item: Clone
Since it only clones the separator, a reference to it would suffice. However, that'd make it a bit more complicated in usage, as it'd always be borrowing something, making it a bit tricky with the lifetime in some situations.
Could we use Borrow<Self::Item> instead?
Borrow is more strict than AsRef and AsRef<Self::Item> probably suffices. However, I'd suggest not to have a generic parameter on intersperse in case we can have intersperse_with, because indirection can be done there, and intersperse's signature remains as simple and straightforward as possible.
I haven't added a function to libs before, so I'm fine with whatever is agreed upon as long as it's reasonable :)
Imho the intersperse as it is now - with minimal signature - has merits; I didn't want to derail this PR, as extensive discussion seems to have taken place. I'd take a look at implementing intersperse_with as mentioned above to accommodate more elaborate use-cases (like Borrow/Into), unless @camelid wants to.
Sorry for the delay. This looks good to me. r=me with the merge conflict resolved.
One comment: Should Intersperse implement FusedIterator if the underlying iterator implements it? (This can be added later, so doesn't need to be resolved now.)
I'd take a look at implementing
intersperse_withas mentioned above
Nice, I'd love to see a PR for that. :)
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
One comment: Should
IntersperseimplementFusedIteratorif the underlying iterator implements it? (This can be added later, so doesn't need to be resolved now.)
That seems reasonable. Like you said, though, we can always add it later. I'll defer to you :)
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
📌 Commit 40bbb7f has been approved by m-ou-se
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
Should there be a doc alias to join since it is similar?
m-ou-se added a commit to m-ou-se/rust that referenced this pull request
Add Iterator::intersperse_with
This is a follow-up to rust-lang#79479, tracking in rust-lang#79524, as discussed rust-lang#79479 (comment).
Note that I had to manually implement Clone and Debug because derive insists on placing a Clone-bound on the struct-definition, which is too narrow. There is a long-standing issue # for this somewhere around here :-)
Also, note that I refactored the guts of Intersperse into private functions and re-used them in IntersperseWith, so I also went light on duplicating all the tests.
If this is suitable to be merged, the tracking issue should be updated, since it only mentions intersperse.
Happy New Year!
r? @m-ou-se
m-ou-se added a commit to m-ou-se/rust that referenced this pull request
Add Iterator::intersperse_with
This is a follow-up to rust-lang#79479, tracking in rust-lang#79524, as discussed rust-lang#79479 (comment).
Note that I had to manually implement Clone and Debug because derive insists on placing a Clone-bound on the struct-definition, which is too narrow. There is a long-standing issue # for this somewhere around here :-)
Also, note that I refactored the guts of Intersperse into private functions and re-used them in IntersperseWith, so I also went light on duplicating all the tests.
If this is suitable to be merged, the tracking issue should be updated, since it only mentions intersperse.
Happy New Year!
r? @m-ou-se
m-ou-se added a commit to m-ou-se/rust that referenced this pull request
m-ou-se added a commit to m-ou-se/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
Labels
Area: Iterators
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library API team, which will review and decide on the PR/issue.