Member

rust-highfive commented on Nov 28, 2020

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

camelid commented on Nov 28, 2020

Squashed.

camelid commented on Nov 28, 2020

r? @LukasKalbertodt (you're the original reviewer, feel free to re-assign)

This comment has been hidden.

This comment has been hidden.

lukaslueg commented on Nov 29, 2020 •

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.

CraftSpider commented on Nov 29, 2020 •

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)

m-ou-se commented on Nov 29, 2020

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.

lukaslueg commented on Nov 29, 2020

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

camelid commented on Nov 30, 2020

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?

lukaslueg commented on Nov 30, 2020

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.

camelid commented on Nov 30, 2020

I haven't added a function to libs before, so I'm fine with whatever is agreed upon as long as it's reasonable :)

lukaslueg commented on Nov 30, 2020

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.

lukaslueg commented on Dec 6, 2020

@m-ou-se love poke

m-ou-se commented 17 days ago

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

m-ou-se commented 17 days ago

I'd take a look at implementing intersperse_with as mentioned above

Nice, I'd love to see a PR for that. :)

camelid commented 16 days ago

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

That seems reasonable. Like you said, though, we can always add it later. I'll defer to you :)

m-ou-se commented 16 days ago

@bors r+

bors commented 16 days ago

Commit 40bbb7f has been approved by m-ou-se

pickfire commented 2 days ago

Should there be a doc alias to join since it is similar?