Skip to content

/rust

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: Clone #23790

Closed
nikomatsakis opened this Issue Mar 27, 2015 · 18 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
  1.0 beta
10 participants
@nikomatsakis @Gankro @alexcrichton @aturon @barosl @tomjakubowski @bombless @Kimundi @freebroccolo @rust-highfive
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 27, 2015

It really makes sense for Clone to be a supertrait of Copy -- Copy is a refinement of Clone where memcpy suffices, basically. I've got a branch handling most of this, though it requires adding a lot of #[derive(Copy, Clone)] sort of things.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 27, 2015

cc @aturon @alexcrichton
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 27, 2015

triage: P-backcompat-lang (1.0 beta)

@rust-highfive rust-highfive added the P-backcompat-lang label Mar 27, 2015

@rust-highfive rust-highfive added this to the 1.0 beta milestone Mar 27, 2015

@Gankro

This comment has been minimized.

Sign in to view
Copy link
Contributor

Gankro commented Mar 27, 2015

💖 ✨ ✨ ✨
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 27, 2015

@Gankro I'm in the tedious process of adding clone impls to run-pass tests right now... woohoo...
@Gankro

This comment has been minimized.

Sign in to view
Copy link
Contributor

Gankro commented Mar 27, 2015

I'm sure this is what has been causing all the spurious buildbot failures.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Mar 27, 2015

though it requires adding a lot of #[derive(Copy, Clone)] sort of things.

Does this mean that we have many Copy things which are not Clone today?
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Mar 27, 2015

I'm very happy with this pragmatic way of resolving the Copy/Clone dichotomy.
@barosl

This comment has been minimized.

Sign in to view
Copy link
Contributor

barosl commented Mar 28, 2015

Does this mean the types with Clone will be implicitly copied, when they were previously moved? Isn't this confusing to move semantics?
@tomjakubowski

This comment has been minimized.

Sign in to view
Copy link
Contributor

tomjakubowski commented Mar 28, 2015

@barosl I don't think so, I think it just means that for a type to impl Copy it must also impl Clone.
@bombless

This comment has been minimized.

Sign in to view
Copy link
Contributor

bombless commented Mar 28, 2015

Yeah I guess #[derive(Copy, Clone)] will keep as-is, while some #[derive(Copy)] will trigger error.
@Kimundi

This comment has been minimized.

Sign in to view
Copy link
Member

Kimundi commented Mar 30, 2015

Sounds like a simple solution, I like it!
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 30, 2015

@alexcrichton

Does this mean that we have many Copy things which are not Clone today?

yes, yes, it does. Not so much in the libraries, I don't think, though there as well.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 30, 2015

@Gankro

I'm sure this is what has been causing all the spurious buildbot failures.

Um, what? (I have no idea what you mean...)

@nikomatsakis nikomatsakis referenced this issue Mar 30, 2015

Merged

Make Copy a subtrait of Clone #23860

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 30, 2015

@alexcrichton see #23860
@freebroccolo

This comment has been minimized.

Sign in to view
Copy link
Contributor

freebroccolo commented Mar 30, 2015

Does it make sense to consider how ToOwned fits into the picture here too? I know it's potentially unrelated but the current interaction between Clone and ToOwned when defining new data structures that hold Cow things is a bit awkward due to impl<T> ToOwned for T where T: Clone.
@bombless

This comment has been minimized.

Sign in to view
Copy link
Contributor

bombless commented Mar 30, 2015

Does it make sense to consider how ToOwned fits into the picture here too?

I think now ToOwned automatically turn &T and &mut T into T (when T: Copy) and that makes sense.
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Mar 30, 2015

@freebroccolo

Does it make sense to consider how ToOwned fits into the picture here too? I know it's potentially unrelated but the current interaction between Clone and ToOwned when defining new data structures that hold Cow things is a bit awkward due to impl<T> ToOwned for T where T: Clone.

I've been vaguely uneasy about the relationship between ToOwned and Clone for a while, but I hadn't quite realized that implication of the blanket impl.

Do you have any thoughts on the direction here? I could imagine removing the blanket impl of ToOwned, although I'm not sure exactly what the fallout would be.
@Gankro

This comment has been minimized.

Sign in to view
Copy link
Contributor

Gankro commented Mar 30, 2015

Niko: just goofin' around! :)

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 1, 2015

@Manishearth
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)
cdcc042

@bors bors closed this in #23860 Apr 2, 2015

actimehl added a commit to actimehl/sodiumoxide that referenced this issue Apr 3, 2015

@actimehl
Fixes in libsodium-sys for rust-lang/rust#23790
dd6ea91

@nikomatsakis nikomatsakis referenced this issue Apr 3, 2015

Closed

Add a lint for types that implement Copy but not Clone #22543

jedisct1 pushed a commit to jedisct1/sodiumoxide that referenced this issue Oct 20, 2015

@actimehl
Fixes in libsodium-sys for rust-lang/rust#23790
52695ac

@nikomatsakis nikomatsakis referenced this issue Mar 2, 2016

Merged

Copy clone semantics #1521

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.