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

Add bindgen::Builder::derive_copy to control whether we emit derive(Copy) on type definitions #948

Closed
ctaggart opened this issue Sep 4, 2017 · 7 comments

Comments

@ctaggart
Copy link

ctaggart commented Sep 4, 2017

I'm getting a lot of errors like the trait Copy may not be implemented for this type, so I went to disable it like I did for .derive_debug, but there isn't an option to turn it off. Shouldn't there be?

    let mut builder = bindgen::Builder::default()
        .header("src/octhc.h")
        // .... snip ...
        .derive_debug(false)
        // .derive_copy(false)
        .whitelist_recursively(false)
        .layout_tests(false)
        .generate_comments(false);
@emilio
Copy link
Contributor

emilio commented Sep 4, 2017

Probably, yeah... Ideally shouldn't be needed. This is basically a dupe of #944.

Right now the analysis landed in #866 assumed that everything not whitelisted should assume to derive copy, when it should be the other way around.

@fitzgen
Copy link
Member

fitzgen commented Sep 5, 2017

It should be pretty straightforward to add a fn derive_copy(mut self, doit: bool) -> Self method to Builder. One could look at how its done for derive_debug to see how to thread the pieces together.

Happy to mentor anyone who wants to take this.

@highfive
Copy link

highfive commented Sep 5, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@fitzgen fitzgen changed the title Builder .derive_copy(false) Add bindgen::Builder::derive_copy to control whether we emit derive(Copy) on type definitions Sep 5, 2017
@fitzgen
Copy link
Member

fitzgen commented Sep 5, 2017

Probably, yeah... Ideally shouldn't be needed. This is basically a dupe of #944.

Right now the analysis landed in #866 assumed that everything not whitelisted should assume to derive copy, when it should be the other way around.

That is certainly a bug, but it also makes sense to allow people to avoid deriving Copy in general. In other words, this isn't a dupe in my opinion.

@mchlrhw
Copy link
Contributor

mchlrhw commented Sep 6, 2017

@fitzgen I'd be happy to help out with this. Just getting into Rust and looking for ways to contribute in my spare time. I've opened a PR for this containing some trivial changes. Will need some help with the rest.

@mchlrhw
Copy link
Contributor

mchlrhw commented Sep 6, 2017

@highfive: assign me

@highfive
Copy link

highfive commented Sep 6, 2017

Hey @mchlrhw! Thanks for your interest in working on this issue. It's now assigned to you!

bors-servo pushed a commit that referenced this issue Sep 6, 2017
Add `bindgen::Builder::derive_copy`

Fixes #948
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

5 participants