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.

Sign up for GitHub

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

[docs] In the documentation of the identify function, clarify that using flatten() is probably a better idea than using filter_map #99230

Closed
wants to merge 3 commits into from
Closed

[docs] In the documentation of the identify function, clarify that using flatten() is probably a better idea than using filter_map #99230

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc65ec9
Clarify that using `flatten()` is probably a better idea in this case
mousetail Jul 14, 2022
9cd38fc
Update mod.rs
mousetail Aug 25, 2022
c4e921f
Typo
mousetail Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions library/core/src/convert/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ pub use num::FloatToInt;
///
/// let iter = [Some(1), None, Some(3)].into_iter();
/// let filtered = iter.filter_map(identity).collect::<Vec<_>>();
/// // Equivalent, since Option implements IntoIterator:
/// // let filtered = iter.flatten().collect::<Vec<_>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option is not an Iterator, but it does implement IntoIterator, which is sufficient for flatten().

Although flatten() is shorter to write, I personally still prefer filter_map(identity) because it is a much simpler iterator, and simple is usually better for optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't really thought about the optimization aspect, just that I prefer not to import more libraries. That makes the advice previously given in the docs not as bad as I thought.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unfortunately .flatten() is a flat_map, not a filter_map, and thus fundamentally more complex. FlatMap is bigger than FilterMap because it needs to store the prefix and suffix iterators, as those might have more elements in them, whereas because FilterMap only deals with Options so knows they can't have another item in them after getting the one value out. (You can also see this by how, for example, filter_map preserves the upper bound on a size_hint, which flat_map doesn't, though admittedly the upper bounds on size hints are useless.) And this is why https://rust-lang.github.io/rust-clippy/master/index.html#flat_map_option exists.

I see that flat_map mentions flatten in the prose, but doesn't have an example -- maybe it'd make sense to move an example like this (perhaps with arrays instead of options) over to that method?

Copy link
Member

@scottmcm scottmcm Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, personally I'd never use identity here, since it needs a use and even ignoring that it isn't even shorter than writing out the |x| x closure. But since it's in the docs for identity and not for filter_map that seems fine.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thank you for your insight, I especially appreciate including references and verifiable facts. I'll try to improve the documentation somewhere else.

I'll close this pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm glad you raised this, since it made me look for a performance lint in clippy talking about this, but instead I found exactly the opposite, a lint suggesting the .filter_map(identity) -> .flatten() change.

So I've filed rust-lang/rust-clippy#9377 for the clippy folks to discuss the situation.

/// assert_eq!(vec![1, 3], filtered);
/// ```
#[stable(feature = "convert_id", since = "1.33.0")]
Expand Down