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

Ensure Default::default is derived when appropriate. #3

Closed
wants to merge 1 commit into from

Conversation

cr1901
Copy link
Contributor

@cr1901 cr1901 commented Apr 12, 2020

Unfortunately, I forgot to derive Default automatically in my initial PR. This PR fixes that for both the pregenerated and compile-time generated bindings.

After this patch, now is probably a good time to publish v0.2.0. I'm starting work on unifying my bindings with yours, and I don't see myself hacking on the pregenerated bindings any extra for now.

@tanriol
Copy link
Owner

tanriol commented Apr 15, 2020

Oh, our derives are actually another interesting problem... I need to look through them, because some of them leave me really unsure (how about Copy on struct ftdi_context?).

@cr1901
Copy link
Contributor Author

cr1901 commented Apr 15, 2020

How about Copy on struct ftdi_context?

It might not make sense, although the original version of the bindings derived Copy as well?

But I'm not particularly attached to deriving everything (and we can always add it back in as a backwards-compat change if necessary after version 1.0).

I needed Default specifically as a smoke test to get ftdi-rs to compile with our shiny new bindings. However, it's not strictly necessary: compare and contrast our constructors!

(I wanted to bikeshed this particular issue on a draft PR on ftdi-rs, but might as well do it now...)

@tanriol
Copy link
Owner

tanriol commented Apr 15, 2020

Yes, I suspect that this was overly optimistic from my side and the way ftdi-rs currently uses worked for me accidentally. Will try to check tomorrow.

@cr1901
Copy link
Contributor Author

cr1901 commented Apr 15, 2020

Will try to check tomorrow.

@tanriol Take your time. For the time being, I'm going to make a draft PR on ftdi-rs, using libftdi1-sys as a git dependency (instead of a crates.io dep), as long as you don't mind the development branch of your libraries requiring the development branch of other libraries. Once we hash out the details, we can replace the git dep with the published 1.0.0 if that works for you!

@tanriol
Copy link
Owner

tanriol commented Apr 16, 2020

...as long as you don't mind the development branch of your libraries requiring the development branch of other libraries...

I don't mind, sure.

I'm going to close this PR as I've looked through the list of structures and I'm pretty sure Default is not appropriate for them, especially for ftdi_context.

@tanriol tanriol closed this Apr 16, 2020
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.

2 participants