Skip to content

Add unstable Iterator::copied() - #56534

Merged
bors merged 10 commits into
rust-lang:masterfrom
sugar700:copied
Dec 26, 2018
Merged

Add unstable Iterator::copied()#56534
bors merged 10 commits into
rust-lang:masterfrom
sugar700:copied

Conversation

@sugar700

@sugar700sugar700 commented Dec 5, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Initially suggested at rust-itertools/itertools#289, however the maintainers of itertools suggested this may be better of in a standard library.

The intent of copied is to avoid accidentally cloning iterator elements after doing a code refactoring which causes a structure to be no longer Copy. This is a relatively common pattern, as it can be seen by calling rg --pcre2 '[.]map[(][|](?:(\w+)[|] [*]\1|&(\w+)[|] \2)[)]' on Rust main repository. Additionally, many uses of cloned actually want to simply Copy, and changing something to be no longer copyable may introduce unnoticeable performance penalty.

Also, this makes sense because the standard library includes [T].copy_from_slice to pair with [T].clone_from_slice.

This also adds Option::copied, because it makes sense to pair it with Iterator::copied. I don't think this feature is particularly important, but it makes sense to update Option along with Iterator for consistency.

@rust-highfive

rust-highfive commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2018
@sugar700
sugar700force-pushed the copied branch 4 times, most recently from 97efb25 to 0800e67CompareDecember 5, 2018 14:24
bluss
bluss reviewed Dec 5, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs Outdated
bluss
bluss reviewed Dec 5, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs Outdated
bluss
bluss reviewed Dec 5, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs
}

fn fold<Acc, F>(self, init: Acc, mut f: F) -> Acc
where F: FnMut(Acc, Self::Item) -> Acc,

@blussblussDec 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the indentation style for where is different in these two methods, we need to use one (whatever this file prefers) 🙂

@sugar700sugar700Dec 5, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It's consistently so in this file, with different indentation for try_fold and fold. It probably should be fixed, but it seems out of scope for this change.

@blussblussDec 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

uh that makes sense I guess 🙂

@LucretielLucretielDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking that it might be good to (at some point) run rustfmt on the whole standard library

@blussblussDec 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's coming

@sugar700sugar700 mentioned this pull request Dec 5, 2018
Merged
bluss
bluss reviewed Dec 5, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs Outdated
bluss
bluss reviewed Dec 5, 2018
View reviewed changes
Comment threadsrc/libcore/iter/mod.rs Outdated
emilyalbini added a commit to emilyalbini/rust that referenced this pull request Dec 5, 2018
@emilyalbini
Rollup merge ofrust-lang#56538- xfix:patch-13, r=bluss
45e1ed8
Use inner iterator may_have_side_effect for Cloned
Previous implementation wasn't correct, as an inner iterator could have had side effects. Noticed by @bluss in rust-lang#56534.
emilyalbini added a commit to emilyalbini/rust that referenced this pull request Dec 5, 2018
@emilyalbini
Rollup merge ofrust-lang#56538- xfix:patch-13, r=bluss
f8ee5ab
Use inner iterator may_have_side_effect for Cloned
Previous implementation wasn't correct, as an inner iterator could have had side effects. Noticed by @bluss in rust-lang#56534.
@Lucretiel

Lucretiel commented Dec 6, 2018

Copy link
Copy Markdown
Contributor

Quick note: there's currently a clippy lint that suggests replacing iter.map(|x| *x) with iter.cloned(). Would probably be worth updating that lint once this API stabilizes.

@kennytmkennytm added the T-libs-api [DEPRECATED; DO NOT USE] label Dec 6, 2018
@SimonSapin

SimonSapin commented Dec 9, 2018

Copy link
Copy Markdown
Contributor

For what it’s worth copy_from_slice is not "just" clone_from_slice with *x instead of x.clone(), it is implemented as a single call to ptr::copy_nonoverlapping.

@sugar700

sugar700 commented Dec 9, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

While true, a similar argument applies to Cloned, as it actually has a specialization for T: Copy for TrustedRandomAccess trait.

That said, the intent of this feature isn't as much about performance, but rather making the invariants clear while refactoring to prevent accidentally cloning, which could be tricky to detect.

@shepmaster

shepmaster commented Dec 16, 2018

Copy link
Copy Markdown
Member

The feature makes sense to me. @bluss has done the existing superb job of review, so I think it's better if they get control of the big button ;-)

r? @bluss

@bors

bors commented Dec 23, 2018

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #57063) made this pull request unmergeable. Please resolve the merge conflicts.

@bluss

bluss commented Dec 23, 2018

Copy link
Copy Markdown
Contributor

Technically the PR is fine. Thoughts on including this in std?

@Centril

Centril commented Dec 24, 2018

Copy link
Copy Markdown
Contributor

(My personal opinion is that this would be useful as a means of ensuring "cheapness")

Randomly re-assigning to someone on T-libs for final triage.

r? @SimonSapin

@SimonSapin

SimonSapin commented Dec 25, 2018

Copy link
Copy Markdown
Contributor

Diff looks good. r=me with a tracking issue filed and referenced in #[unstable] attributes.

I believe we tend to default to accepting new unstable APIs, and revisit them when stabilization is proposed.

@SimonSapinSimonSapin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 25, 2018
This was referenced Dec 26, 2018
Closed
Closed
@sugar700

sugar700 commented Dec 26, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@SimonSapin Created tracking issues.

@Centril

Centril commented Dec 26, 2018

Copy link
Copy Markdown
Contributor

@bors r=@SimonSapin

@bors

bors commented Dec 26, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 315401d has been approved by @SimonSapin

@bors

bors commented Dec 26, 2018

Copy link
Copy Markdown
Collaborator

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2018
@bors

bors commented Dec 26, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 315401d with merge a7be40c...

bors added a commit that referenced this pull request Dec 26, 2018
@bors
Auto merge of#56534- xfix:copied, r=@SimonSapin
a7be40c
Add unstable Iterator::copied()
Initially suggested at rust-itertools/itertools#289, however the maintainers of itertools suggested this may be better of in a standard library.
The intent of `copied` is to avoid accidentally cloning iterator elements after doing a code refactoring which causes a structure to be no longer `Copy`. This is a relatively common pattern, as it can be seen by calling `rg --pcre2 '[.]map[(][|](?:(\w+)[|] [*]\1|&(\w+)[|] \2)[)]'` on Rust main repository. Additionally, many uses of `cloned` actually want to simply `Copy`, and changing something to be no longer copyable may introduce unnoticeable performance penalty.
Also, this makes sense because the standard library includes `[T].copy_from_slice` to pair with `[T].clone_from_slice`.
This also adds `Option::copied`, because it makes sense to pair it with `Iterator::copied`. I don't think this feature is particularly important, but it makes sense to update `Option` along with `Iterator` for consistency.
@bors

bors commented Dec 26, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: @SimonSapin
Pushing a7be40c to master...

@bors
bors merged commit 315401d into rust-lang:masterDec 26, 2018
@borsbors mentioned this pull request Dec 26, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@LucretielLucretielLucretiel left review comments
@blussblussbluss left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@SimonSapinSimonSapin

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@sugar700@rust-highfive@Lucretiel@SimonSapin@shepmaster@bors@bluss@Centril@kennytm