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

remove Clone trait implementation and use #[derive(Clone)] #34130

Closed
wants to merge 1 commit into from
Closed

remove Clone trait implementation and use #[derive(Clone)] #34130

wants to merge 1 commit into from

Conversation

srinivasreddy
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@brson
Copy link
Contributor

brson commented Jun 7, 2016

@bors r+ rollup. Thanks!

@bors
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit 6a4c97b has been approved by brson

@eefriedman
Copy link
Contributor

Hang on, are you sure this is right? I thought the reason these were derived by hand is that derive(Clone) doesn't use the right bounds; as far as I know that hasn't been fixed.

@durka
Copy link
Contributor

durka commented Jun 7, 2016

@eefriedman is correct -- this adds T: Clone bounds to the Clone impls that were not there before.

@apasel422
Copy link
Contributor

@bors r-

@srinivasreddy
Copy link
Contributor Author

I want to implement this correct. Pointers please.

@durka
Copy link
Contributor

durka commented Jun 7, 2016

@eddyb had thoughts about fixing this situation by elaborating trait bounds before the privacy checks. The problem is that #[derive(Clone)] needs to bound the impl by all the fields implementing Clone. But currently it can't simply list all the field types in the where clause, because it might get a "private type in public interface". So it goes with a sometimes-too-liberal strategy of bounding all type parameters. See #31518 and #33769.

@alexcrichton
Copy link
Member

Yeah this is implemented correctly as-is to avoid the extra Clone bound on type parameters. There isn't really a "more correct" way to do this because this is indeed the "correct way to do this". As a result I'm gonna close this, but thanks for the PR!

@durka
Copy link
Contributor

durka commented Jun 7, 2016

Well, a more correct way would be making the derived impls better. But that
can't be done without compiler support in some way.

On Tue, Jun 7, 2016 at 1:54 AM, Alex Crichton notifications@github.com
wrote:

Yeah this is implemented correctly as-is to avoid the extra Clone bound
on type parameters. There isn't really a "more correct" way to do this
because this is indeed the "correct way to do this". As a result I'm gonna
close this, but thanks for the PR!


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34130 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC3n7h_nN6ealQ9QvmB1FzDXFfr8eQeks5qJQeOgaJpZM4Ivci1
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants