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 missing Copy/Clone impls for Iterators #47304

Closed
wants to merge 1 commit into from

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Jan 10, 2018

There's a bit of an open question here that probably requires some libs/lang team discussion: do we want to implement Copy for Iterator types, or are the ergonomic and performance benefits outweighed by the potential for confusion? @nikomatsakis previously proposed introducing a Copy-lint that would warn against types that usually wouldn't be Copyd. Personally, I think we should implement Copy for nearly any type possible to allow users the flexibility to make their own iterator-containing types Copy where appropriate.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@eddyb
Copy link
Member

eddyb commented Jan 10, 2018

The Copy iterator discussion has a messy past, please don't combine it with fixing the calendar example (which should be trivial wrt Copy closures)

@cramertj
Copy link
Member Author

@eddyb Sure-- I'll split it out. Sorry, I think there's some history here that I'm not aware of.

@cramertj cramertj changed the title Calendar-Driven Development: Add missing Copy/Clone impls for Iterators Add missing Copy/Clone impls for Iterators Jan 10, 2018
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2018
@Mark-Simulacrum
Copy link
Member

I'm inclined to say that it's unlikely we'll accept Copy iterators at this point -- from past discussion(s) the current consensus seems to be that it's too error prone to have them since accidentally copying is too easy. That may have since changed, though.

@eddyb
Copy link
Member

eddyb commented Jan 10, 2018

@Mark-Simulacrum I thought there was a consensus that if we add lints we can get away with it.

@kennytm
Copy link
Member

kennytm commented Jan 10, 2018

Could we impl Copy for Range if that's the case? 🤩

@Mark-Simulacrum
Copy link
Member

Not sure. Seems okay, and probably doesn't affect too many crates in practice. I'm okay with this, but do think we should get consensus before going ahead with it from at least some of the libs team.

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 10, 2018
@eddyb
Copy link
Member

eddyb commented Jan 10, 2018

@kennytm Well, yes, but again, it's predicated on someone actually writing those lints.
They might require operating on MIR (which recently gained lint information, so we could do it now), but I don't recall whether we know when we'd emit warnings (cc @nikomatsakis).

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 10, 2018

I think we should add some sort of lint. It's not that hard. Then let's aggressively add Copy.

I had previously opened #45683 for tracking that. I can put up some kind of simple, first-step proposal.

kennytm added a commit to kennytm/rust that referenced this pull request Jan 11, 2018
…ddyb

Use copy/clone closures to simplify calendar test

Split out from rust-lang#47304

r? @eddyb
kennytm added a commit to kennytm/rust that referenced this pull request Jan 11, 2018
…ddyb

Use copy/clone closures to simplify calendar test

Split out from rust-lang#47304

r? @eddyb
kennytm added a commit to kennytm/rust that referenced this pull request Jan 12, 2018
…ddyb

Use copy/clone closures to simplify calendar test

Split out from rust-lang#47304

r? @eddyb
@cramertj
Copy link
Member Author

Closing to keep the PR set clean until we've got the appropriate lint in place.

@cramertj cramertj closed this Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants