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

Make Copy a subtrait of Clone #23860

Merged
merged 7 commits into from Apr 2, 2015

Conversation

Projects
None yet
7 participants
@nikomatsakis
Copy link
Contributor

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 referenced this pull request Mar 30, 2015

Closed

make Copy: Clone #23790

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:copy-requires-clone branch from e340206 to a608750 Mar 30, 2015

@@ -8,10 +8,18 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

<<<<<<< HEAD

This comment has been minimized.

@pnkfelix

pnkfelix Mar 30, 2015

Member

this can't be right.

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 30, 2015

Author Contributor

I would guess not

@@ -227,7 +227,7 @@ pub mod types {
pub type rlim_t = u64;

#[repr(C)]
#[derive(Copy)] pub struct glob_t {
#[derive(Copy, Clone)] pub struct glob_t {

This comment has been minimized.

@alexcrichton

alexcrichton Mar 30, 2015

Member

Hm, the fallout in FFI types is... somewhat unfortunate!

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 30, 2015

Author Contributor

@alexcrichton why? (that is, why is this any more unfortunate than any other fallout?)

seems like this will be a lot less painful for everyone if we made #[derive(Copy)] imply #[derive(Clone)]...

This comment has been minimized.

@alexcrichton

alexcrichton Mar 30, 2015

Member

I personally prefer to not see inherent methods and trait implementations directly on FFI types but rather on appropriate wrappers, so this is forcing the addition of a trait implementation on FFI types that want to also be Copy. Although others may differ on that opinion!

I do agree that inferred derive would be nice.

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 30, 2015

Author Contributor

I see. Seems like a minor thing to me though. You can still avoid trait impls beyond Copy/Clone if you choose.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 30, 2015

r=me once bad merge is resolved.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 30, 2015

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

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:copy-requires-clone branch to eb8315a Mar 30, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Mar 30, 2015

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:copy-requires-clone branch from eb8315a to 32b6ec6 Mar 30, 2015

@theemathas

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Mar 31, 2015

@bors r=aturon 32b6ec6

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Mar 31, 2015

oh, needs merge.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:copy-requires-clone branch from 32b6ec6 to a758cf5 Mar 31, 2015

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 31, 2015

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r= aturon 1b5ac16

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 1, 2015

📌 Commit 1b5ac16 has been approved by ``

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r=aturon 1b5ac16

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:copy-requires-clone branch from 1b5ac16 to c35c468 Apr 1, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r=aturon c35c468

@Manishearth

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

@bors r=aturon 943729f

@nikomatsakis

This comment has been minimized.

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 added a commit to Manishearth/rust that referenced this pull request Apr 1, 2015

Rollup merge of rust-lang#23860 - nikomatsakis:copy-requires-clone, r…
…=aturon

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:

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

Fixes rust-lang#23790.

r? @aturon (stabilization issue)

bors added a commit that referenced this pull request Apr 1, 2015

@Manishearth

This comment has been minimized.

bors added a commit that referenced this pull request Apr 1, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Apr 1, 2015

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

@bors

This comment has been minimized.

Copy link
Contributor

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

rollup merge of rust-lang#23860: nikomatsakis/copy-requires-clone
Conflicts:
	src/test/compile-fail/coherence-impls-copy.rs
@alexcrichton

This comment has been minimized.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

andersk added a commit to andersk/image-rs that referenced this pull request Apr 3, 2015

Derive Clone when deriving Copy
As per rust-lang/rust#23860, Copy now extends
Clone.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

@CraZySacX CraZySacX referenced this pull request Apr 3, 2015

Merged

Fd ist now RawFd (rustup) #92

andersk added a commit to andersk/image-rs that referenced this pull request Apr 3, 2015

Derive Clone when deriving Copy
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

@zmack zmack referenced this pull request Apr 5, 2015

Closed

Compatibility with rust latest #94

jooert added a commit to jooert/rust-bswap that referenced this pull request Apr 29, 2015

Update code to compile with latest rustc.
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 added a commit to jooert/rust-bswap that referenced this pull request Apr 29, 2015

Update code to compile with latest rustc.
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).

0xbrt added a commit to 0xbrt/rust-alsa that referenced this pull request Jun 17, 2015

Derive `Clone` for copyable types
Copy is a subtrait of Clone (see rust-lang/rust#23860), meaning
memcopyable types must derive from Clone as well as Copy.

@nikomatsakis nikomatsakis deleted the nikomatsakis:copy-requires-clone branch Mar 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.