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

Support for TryFrom::try_from on the items of iterator #913

Open
schneiderfelipe opened this issue Apr 12, 2024 · 5 comments
Open

Support for TryFrom::try_from on the items of iterator #913

schneiderfelipe opened this issue Apr 12, 2024 · 5 comments

Comments

@schneiderfelipe
Copy link

It would be nice to have a map_try_into similar to map_into already implemented (in #288, requested in #280). The signature would be something like

fn map_try_into<T>(self) -> impl Iterator<Item = Result<T, <Self::Item as TryInto<T>>::Error>>
where
    Self::Item: TryInto<T>,
{
    std::iter::from_fn(|| todo!())
}
@Philippe-Cholet
Copy link
Member

No doubt it's easily doable by copying the code we have for MapInto and merely update it.

The alternative is .map(TryInto::try_into). "Is such addition worth it?" is the main question.
And where do stop with map_-prefixed methods? Because I can imagine a bunch of those, like .map_expect(msg) for .map(|result| result.expect(msg)).
I kinda wonder what's the difference of generated codes for .map_into() vs .map(Into::into). Does it optimize similarily well?

@scottmcm
Copy link
Contributor

In general, I'd expect custom map iterators to actually optimize worse, because they can't take advantage of all the specializations in the standard library.

For example, std::iter::Map is https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/std/iter/trait.TrustedLen.html if the original iterator is TrustedLen, but iterators in itertools can't be.

So I would say that itertools shouldn't have any of the .map(thing) iterators. It's just not that hard to write .map(Into::into) or whatever.

@Philippe-Cholet
Copy link
Member

Apart from not making map_try_into...
@jswrenn @phimuemue Should we consider deprecate .map_into() in favor of .map(Into::into)?

@jswrenn
Copy link
Member

jswrenn commented Apr 18, 2024

Agree with @scottmcm that we shouldn't be adding more trivial shorthands that don't optimize well. We should revisit this decision when type_alias_impl_trait is stabilized and RPITIT is usable on our MSRV: those will let us easily define short-hands that optimize as well as if they had been in the standard library.

In the meantime, I don't think we need to go so far as deprecating map_into; a documentation note on performance might suffice.

@scottmcm
Copy link
Contributor

RPITIT is a great call-out here. If these could be written as just -> std::iter::Map<impl Fn<…> + Copy> or whatever, things could well be different. (Though even then I'm not convinced that .map_try_into() is necessarily better than .map(TryInto::try_into) or .map(|x| x.try_into()), because there's cost to remembering all the custom things too.)

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

No branches or pull requests

4 participants