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

Derive Clone when deriving Copy #934

Closed
dimbleby opened this issue Aug 26, 2017 · 7 comments
Closed

Derive Clone when deriving Copy #934

dimbleby opened this issue Aug 26, 2017 · 7 comments

Comments

@dimbleby
Copy link
Contributor

bindgen almost always implements Clone explicitly, like this:

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

I (accidentally!) ran clippy over some autogenerated code and it expressed disappointment that Clone was manually implemented for all sorts of structures on which Copy was derived:

warning: you are implementing `Clone` explicitly on a `Copy` type

And indeed, Clone could be derived in every case, at least for my project.

The code that prevents this is here.

My input header is a plain C header (so certainly no templates). If anything, it almost looks as though that test is exactly backwards, though I expect it's more likely that I simply don't understand what issue is being avoided here...

Anyway, it certainly seems like Clone could be derived a lot more often than it currently is.

@emilio
Copy link
Contributor

emilio commented Aug 26, 2017

This is pretty much intentional, because of large arrays, which implement Copy but not Clone. I'm not sure it's worth to add the complexity to derive it when possible tbh.

@dimbleby
Copy link
Contributor Author

Well, certainly this is at worst a wart rather than a bug. I don't really expect auto-generated code to be clean under clippy. (But wouldn't it be nice if it were?).

Compared to the work you must already do figuring out when all the other derived things are derivable, I'd have guessed that "is there a big array involved?" was a relatively straightforward check... but that's absolutely a guess from ignorance, and I'm happy to be contradicted by someone who actually knows what they're talking about!

So yeah, I'm content to defer to maintainer judgement here. If you think that the effort / reward ratio for this one will never be worthwhile, please just close it out.

@fitzgen
Copy link
Member

fitzgen commented Sep 18, 2017

This is pretty much intentional, because of large arrays, which implement Copy but not Clone. I'm not sure it's worth to add the complexity to derive it when possible tbh.

Is this the only case where we end up manually implementing Clone? The source comments look like it has more to do with type parameters.

@dimbleby
Copy link
Contributor Author

Is this the only case where we end up manually implementing Clone?

No: Clone is manually implemented nearly always so far as I can tell, even when no arrays (large or otherwise) are involved.

@fitzgen
Copy link
Member

fitzgen commented Sep 18, 2017

Huh. This doesn't sound right. It seems like I don't have the full picture here, and we need some more investigation.

@dimbleby
Copy link
Contributor Author

IIUC, as of Rust 1.21 and rust-lang/rust#43690, large arrays should no longer be an issue when deriving Clone.

  • [T; N] is now Clone if T: Clone (currently, only if T: Copy and for N <= 32).

@fitzgen
Copy link
Member

fitzgen commented Oct 13, 2017

Awesome :)

We will need to add a new RustTarget for 1.21 and take that into account in our analyses/codegen.

bors-servo pushed a commit that referenced this issue Oct 24, 2017
Derive `Clone` along with `Copy` on Rust 1.21

Fixes #934

r? @fitzgen or @emilio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants