KEMBAR78
Implement `Default` for `sys` style structs by kennykerr · Pull Request #3520 · microsoft/windows-rs · GitHub
Skip to content

Implement Default for sys style structs #3520

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 27, 2025
Merged

Implement Default for sys style structs #3520

merged 4 commits into from
Feb 27, 2025

Conversation

kennykerr
Copy link
Collaborator

The --sys style bindings generated by windows-bindgen implement fewer standard traits to minimize code generation and build time. A few are required and Default is simple enough that it does not appear to impact build time or complexity. So this update adds a Default implementation for structs. Deciding when this can be derived is however more complicated as so many more types decay to pointers in --sys style bindings.

@kennykerr
Copy link
Collaborator Author

It expands the size of the windows-sys crate so that's not great, but I'm more concerned about making windows-bindgen more broadly useful so perhaps this is worthwhile. Long term I think it makes more sense for developers to use windows-bindgen directly.

Thoughts? @dpaoliello @ChrisDenton

dpaoliello
dpaoliello approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@dpaoliello dpaoliello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the increase in the crate size is worth it - it both makes the Rust bindings more useful and makes it easier to port C code.

ChrisDenton
ChrisDenton approved these changes Feb 26, 2025
View reviewed changes
Copy link
Collaborator

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing the amount of unsafe code end users end up writing is a good thing in my book. Especially when it's trivially safe (and technically could be written in safe code but few are going to bother). Default is also just so much more convenient.

@riverar
Copy link
Collaborator

riverar commented Feb 26, 2025

I'd argue this is moving away from the convention of -sys crates being minimal thin FFI bindings without additional abstractions or traits. But not going to die on that hill for a crate that will probably go away in the future.

@kennykerr
Copy link
Collaborator Author

Ideally there'd be no need for two different modes of code generation but there are some obvious benefits right now so at the very least I'd like to avoid unnecessary differences that make it harder to use one or the other and this seems like one of those.

@kennykerr kennykerr merged commit 1d0fbe0 into master Feb 27, 2025
34 checks passed
@kennykerr kennykerr deleted the sys-default branch February 27, 2025 13:59
This was referenced Mar 11, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrisDenton ChrisDenton ChrisDenton approved these changes

@dpaoliello dpaoliello dpaoliello approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kennykerr @riverar @ChrisDenton @dpaoliello