Skip to content

Make Copy a subtrait of Clone#23860

Merged
bors merged 7 commits intorust-lang:masterfrom
nikomatsakis:copy-requires-clone
Apr 2, 2015
Merged

Make Copy a subtrait of Clone#23860
bors merged 7 commits intorust-lang:masterfrom
nikomatsakis:copy-requires-clone

Conversation

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 30, 2015

Logically, Copy: Clone makes sense, since Copy basically means "I can clone this with just memcpy". This also means that one can start out with a Copy bound and relax it to a Clone bound.

This is a [breaking-change] because one must change #[derive(Copy)] to #[derive(Copy,Clone)]. In some cases, deriving doesn't create a proper clone impl (notably around fixed-length arrays), in which case you should write one by hand. Because the type in question is Copy, this is very simple:

impl Clone for Foo { fn clone(&self) -> Foo { *self } }

Fixes #23790.

r? @aturon (stabilization issue)

@nikomatsakis nikomatsakis mentioned this pull request Mar 30, 2015
Closed
pnkfelix
pnkfelix reviewed Mar 30, 2015
View reviewed changes
src/test/run-pass/guards-not-exhaustive.rs Outdated
Copy link
Contributor

@pnkfelix pnkfelix Mar 30, 2015

Choose a reason for hiding this comment

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

this can't be right.

Copy link
Contributor Author

@nikomatsakis nikomatsakis Mar 30, 2015

Choose a reason for hiding this comment

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

I would guess not

@aturon
Copy link
Contributor

aturon commented Mar 30, 2015

r=me once bad merge is resolved.

@bors
Copy link
Collaborator

bors commented Mar 30, 2015

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

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 30, 2015

Created a discuss thread about this PR http://internals.rust-lang.org/t/making-copy-a-subtrait-of-clone-pr-23860/1777

@theemathas
Copy link
Contributor

theemathas commented Mar 31, 2015

I have always assumed that Copy was not a subtrait of Clone because of &T.

Turns out that &T is both Copy and Clone, after all.

👍 😃

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 31, 2015

@bors r=aturon 32b6ec6

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 31, 2015

oh, needs merge.

@bors
Copy link
Collaborator

bors commented Mar 31, 2015

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

@erickt erickt mentioned this pull request Mar 31, 2015
Closed
@nikomatsakis nikomatsakis force-pushed the copy-requires-clone branch from a758cf5 to 1b5ac16 Compare April 1, 2015 15:19
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r= aturon 1b5ac16

@bors
Copy link
Collaborator

bors commented Apr 1, 2015

📌 Commit 1b5ac16 has been approved by ``

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r=aturon 1b5ac16

@nikomatsakis nikomatsakis force-pushed the copy-requires-clone branch from 1b5ac16 to c35c468 Compare April 1, 2015 15:23
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r=aturon c35c468

@Manishearth
Copy link
Member

Manishearth commented Apr 1, 2015

Would this bloat crate size for crates that derive Copy on exported types but not Clone?

I'm also a bit concerned if this affects #![no_std] since Copy is a lang item.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r=aturon 943729f

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@Manishearth presumably this will affect crate sizes, specifically metadata. As for #![no_std], avoiding the core crate feels to me like it's kind of "extra unstable" -- you're just asking for trouble, given the number of lang items and intrinsics that are in there.

Bottom line for me though is that making Copy: Clone is really the right call for the language in multiple ways, whereas problems of binary size etc feel like artifacts of current implementation.

@Manishearth Manishearth mentioned this pull request Apr 1, 2015
Closed
@Manishearth
Copy link
Member

Manishearth commented Apr 1, 2015

http://buildbot.rust-lang.org/builders/auto-win-32-nopt-t/builds/3983 ?

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@Manishearth that should be fixed by the most recent push.

@bors
Copy link
Collaborator

bors commented Apr 2, 2015

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

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 2, 2015
@alexcrichton
rollup merge of rust-lang#23860: nikomatsakis/copy-requires-clone
f92e7ab 
Conflicts:
	src/test/compile-fail/coherence-impls-copy.rs
@alexcrichton
Copy link
Member

alexcrichton commented Apr 2, 2015

gah I missed this in the rollup!

@bors bors merged commit 4496433 into rust-lang:master Apr 2, 2015
@semarie semarie mentioned this pull request Apr 2, 2015
Merged
@alexcrichton alexcrichton mentioned this pull request Apr 2, 2015
Closed
andersk added a commit to andersk/image-rs that referenced this pull request Apr 3, 2015
@andersk
Derive Clone when deriving Copy
ef044ab 
As per rust-lang/rust#23860, Copy now extends
Clone.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@CraZySacX CraZySacX mentioned this pull request Apr 3, 2015
Merged
andersk added a commit to andersk/image-rs that referenced this pull request Apr 3, 2015
@andersk
Derive Clone when deriving Copy
ea58de3 
As per rust-lang/rust#23860, Copy now extends
Clone.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
DaGenix added a commit to DaGenix/rust-crypto that referenced this pull request Apr 4, 2015
@DaGenix
Merge pull request #290 from rustyhorde/master
734248d 
Fixes for change introduced by rust-lang/rust#23860
@zmack zmack mentioned this pull request Apr 5, 2015
Closed
@justinlatimer justinlatimer mentioned this pull request Apr 7, 2015
Merged
jooert pushed a commit to jooert/rust-bswap that referenced this pull request Apr 29, 2015
Update code to compile with latest rustc.
b7c143a 
Swap order of input and output parameters of ptr::copy_nonoverlapping
(rust-lang/rust#23866), remove usage of
std::num::Int (rust-lang/rust#23549), change
(rust-lang/rust#23860).
jooert pushed a commit to jooert/rust-bswap that referenced this pull request Apr 29, 2015
Update code to compile with latest rustc.
637dae4 
Swap order of input and output parameters of ptr::copy_nonoverlapping
(rust-lang/rust#23866), remove usage of
std::num::Int (rust-lang/rust#23549), change
(rust-lang/rust#23860).
@jooert jooert mentioned this pull request Apr 29, 2015
Closed
@mitaa mitaa mentioned this pull request Jul 13, 2015
Closed
@nikomatsakis nikomatsakis deleted the copy-requires-clone branch March 30, 2016 16:12
BlockBlazeDev added a commit to BlockBlazeDev/rust-crypto that referenced this pull request Aug 8, 2024
@BlockBlazeDev
Fixes for rust-lang/rust#23860
856511c
BlockBlazeDev added a commit to BlockBlazeDev/rust-crypto that referenced this pull request Aug 8, 2024
@BlockBlazeDev
Fixes for rust-lang/rust#23860
ca5a652
BlockBlazeDev added a commit to BlockBlazeDev/rust-crypto that referenced this pull request Aug 8, 2024
@BlockBlazeDev
Merge pull request #290 from rustyhorde/master
132bf7d 
Fixes for change introduced by rust-lang/rust#23860
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@aturon aturon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

make Copy: Clone

7 participants

@nikomatsakis @aturon @bors @theemathas @Manishearth @alexcrichton @pnkfelix