-
Notifications
You must be signed in to change notification settings - Fork 276
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
Proposed fixes to build with MinGW-w64 8.0.0 and JWasm #475
Conversation
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.
Mostly looks good. I noticed one thing that I didn't catch before which I think would be best to tweak, which I highlighted inline. I can change that or you can — it doesn't matter to me. I'm running this through our internal CI and I'll update when that passes or fails.
loader/loader.c
Outdated
const ULONG flags = CM_GETIDLIST_FILTER_CLASS | CM_GETIDLIST_FILTER_PRESENT; | ||
#else | ||
const ULONG flags = CM_GET_DEVICE_INTERFACE_LIST_PRESENT; |
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.
This is pretty clearly clear intended for the CM_Get_Device_Interface_List
functions whereas we're using this in the CM_Get_Device_ID_List
functions so I don't think this is quite correct. Interestingly, the value for this is zero, which is equivalent to CM_GETIDLIST_FILTER_NONE
so it probably works ok. But that does mean you're not filtering out devices that have been removed (which is half the point we added this code). I think the solution I'd go with is probably just to add the numerical value instead so the behavior doesn't change. So I'd change this to:
const ULONG flags = 0x300; // Equivalent to (CM_GETIDLIST_FILTER_CLASS | CM_GETIDLIST_FILTER_PRESENT) which aren't always defined on MinGW
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.
Yes, that makes more sense.
My attempt was to find the equivalent in MinGW-w64, but if it results in 0 that doesn't make sense.
Go ahead and change it if you like.
See http://winlibs.com for a release of such GCC compiler See issue #474
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.
Sorry for the delay. I just made the change I described and I tweaked the commit message to better match this repo's format while I was at it. Looks good now.
Taken from KhronosGroup/Vulkan-Loader#475. Supersedes and reverts godotengine#43119 since the upstream change removes the need for that custom define.
#474
See http://winlibs.com for a release of a GCC compiler with MinGW-w64 8.0.0.