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.

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

Add Itertools::intersperse_with #381

Merged
merged 3 commits into from Jun 18, 2020

Conversation

gin-ahirsch
Copy link
Contributor

Pretty self-explanatory. Useful for non-clonable elements or otherwise stateful interspersing.
intersperse() could be implemented via intersperse_with(), allowing the change to be more minimal. I haven't done it currently, but if you don't see any downsides I'd amend this change to do self.intersperse_with(|| element) for intersperse() instead.

@FauxFaux
Copy link

I came here to write exactly this.

I planned to:

  • make the existing intersperse implementation call this one. Copy-pasting the code is presumably more efficient, though.
  • not use FnMut, but Fn. I guess FnMut is more general.

@phimuemue
Copy link
Member

make the existing intersperse implementation call this one.

I second this suggestion. We do it in other places, too, and my most recent tests indicate that the compiler is actually smart enough to inline things properly (at least with --release).

As a side note: Is possibly even Interleave similar enough so that it could share code with Intersperse?

@gin-ahirsch
Copy link
Contributor Author

It may not actually be possible to implement intersperse() through intersperse_with():

error[E0308]: mismatched types
   --> src/lib.rs:401:31
    |
396 |     fn intersperse<F>(self, element: Self::Item) -> IntersperseWith<Self, F>
    |                    - this type parameter
...
401 |         self.intersperse_with(|| element)
    |                               ^^^^^^^^^^ expected type parameter `F`, found closure

Basically the same underlying issue as why #377 can't just return with a new closure - requiring impl Trait in return position.
The same will be true for interleave().

@gin-ahirsch
Copy link
Contributor Author

Actually the issue is not just impl Trait in return position, but impl Trait in return position for traits. I'm not sure this is even on the map for Rust, as a Trait's interface is expected to be "fixed".
I tried using inherent trait methods (impl dyn Itertools), but that doesn't like methods with generic type parameters that don't have the restriction of Self: Sized:

error[E0038]: the trait `Itertools` cannot be made into an object
    --> src/lib.rs:2590:9
     |
1394 |     fn find_position<P>(&mut self, mut pred: P) -> Option<(usize, Self::Item)>
     |        ------------- method `find_position` has generic type parameters
...
2590 | impl<Item> dyn Itertools<Item=Item> {
     |            ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Itertools` cannot be made into an object

I'm not sure why that would be problematic if not invoked from the inherent methods, but it seems that it is.
Even if that would be solved somehow, we cannot invoke generic methods at all though:

error: the `intersperse_with` method cannot be invoked on a trait object
    --> src/lib.rs:2467:14
     |
2467 |         self.intersperse_with(|| element)
     |              ^^^^^^^^^^^^^^^^

It's interesting that the method can accept self by value though, considering its type would be dyn Itertools.

@jswrenn
Copy link
Member

jswrenn commented May 3, 2020

This looks good to me. Could you resolve the merge conflict?

@jswrenn
Copy link
Member

jswrenn commented Jun 18, 2020

Thanks!

bors r+

@jswrenn jswrenn added this to the next milestone Jun 18, 2020
@bors
Copy link
Contributor

bors bot commented Jun 18, 2020

Build succeeded:

@bors bors bot merged commit 636a332 into rust-itertools:master Jun 18, 2020
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.

None yet

4 participants