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

add perf side effect docs to Iterator::cloned() #92955

Merged
merged 1 commit into from
Mar 23, 2022
+16 −0
Merged

add perf side effect docs to Iterator::cloned() #92955

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions library/core/src/iter/traits/iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3189,6 +3189,10 @@ pub trait Iterator {
/// This is useful when you have an iterator over `&T`, but you need an
/// iterator over `T`.
///
/// There is no guarantee whatsoever about the `clone` method actually
/// being called *or* optimized away. So code should not depend on
/// either.
///
Copy link
Member

@the8472 the8472 Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This one seems a little more troublesome to me. I don't think this adds anything new, it simply makes the "no guarantee in either direction" which was previously implicit in the absence of detailed specification explicit. But some libs team members interpret API contracts differently than I do, so I want some extra eyes on this. Wait, I'm not assigned reviewer, nevermind 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least making it explicit allows us to point the user to the docs when they bemoan their code breaking should we ever decide to improve perf on this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind making the lack of a guarantee explicit, I don't believe this restricts us in any way which is the main thing I worry about with docs changes

/// [`clone`]: Clone::clone
///
/// # Examples
Expand All @@ -3206,6 +3210,18 @@ pub trait Iterator {
/// assert_eq!(v_cloned, vec![1, 2, 3]);
/// assert_eq!(v_map, vec![1, 2, 3]);
/// ```
///
/// To get the best performance, try to clone late:
llogiq marked this conversation as resolved.
Show resolved Hide resolved
///
/// ```
/// let a = [vec![0_u8, 1, 2], vec![3, 4], vec![23]];
/// // don't do this:
/// let slower: Vec<_> = a.iter().cloned().filter(|s| s.len() == 1).collect();
/// assert_eq!(&[vec![23]], &slower[..]);
/// // instead call `cloned` late
/// let faster: Vec<_> = a.iter().filter(|s| s.len() == 1).cloned().collect();
/// assert_eq!(&[vec![23]], &faster[..]);
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

I think this part should be fairly uncontroversial (modulo readability nits), it simply adds an implementation hint.

#[stable(feature = "rust1", since = "1.0.0")]
fn cloned<'a, T: 'a>(self) -> Cloned<Self>
where
Expand Down