Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Regenerate windows sys bindings #1132
Regenerate windows sys bindings #1132
Changes from 6 commits
f9c9193
c407c89
6dfd7f6
0b408d3
f6ed3de
f394c9f
cbefede
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the import library is often named the same as the DLL, this isn't always true (e.g. there's a Synchronization.lib but not synchronization.dll). This doesn't affect cc-rs at the moment but it could do in the future. I wonder if it would be better to maintain a manual mapping, even if this is more verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many of these cases are there?
If there're only a few, then I could hard code it here, so that people don't have to modify the
macro_rules!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Windows API is sprawling so it's hard to answer definitely but it seems like there are a lot. Complicating matters further are functions like
GetFileVersionInfoExA
which is exported from version.dll and there is version.lib but the lib doesn't contain that function. I don't know how many of them there are but cases like that would need to be handled specially by matching on the function name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could bump the msrv up to 1.65 and just use raw-dylib. That would completely side-step any issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with raw-dynlib, but the problem is that we can't enable it just cc-rs, it's not a feature but a cfg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think it's fine for now, it's not like we add new windows sys binding very often.
Thank you so much, and I will merge it as now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a cfg if we implement in a macro ourselves. Then it just a
#[link ... kind="raw-dylib"]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I didn't know we could do that, would open another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example: