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

Explicitly mark compareLayer as __cdecl #231

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

jenatali
Copy link
Contributor

@jenatali jenatali commented Feb 5, 2024

There's command-line switches which can change the default calling convention for unannotated functions. Visual Studio defaults to /Gd which is __cdecl, but Windows defaults to /Gz which is __stdcall. That causes an x86 build break, since qsort requires the input parameter to be __cdecl. Just add the explicit annotation so it works regardless of the default.

Copy link
Contributor

@Kerilk Kerilk left a comment

Choose a reason for hiding this comment

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

Nice one, thanks for fixing this! Once CI succeeds, we should be able to merge.

@jenatali
Copy link
Contributor Author

jenatali commented Feb 5, 2024

Looks like the CI run hit an infrastructure issue (timeout retrieving GPG key). I don't see an option to re-run it but I'm not super familiar with the GitHub actions UI. Might just be a permission issue though.

@Kerilk
Copy link
Contributor

Kerilk commented Feb 5, 2024

Launched a rerun of the failed jobs.

@Kerilk
Copy link
Contributor

Kerilk commented Feb 6, 2024

@bashbaug @MathiasMagnus Do we want to add the above configuration to the CI? Or merge this as is and try to remember about this?

@MathiasMagnus
Copy link
Contributor

This seems like a good change. We should probably test both calling conventions, but I'd defer it via an issue of its own.

@bashbaug
Copy link
Contributor

bashbaug commented Feb 7, 2024

I added an issue to track adding this case to CI, so we don't lose track of it. Let's just merge this now.

@bashbaug bashbaug merged commit 5383646 into KhronosGroup:main Feb 7, 2024
179 checks passed
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.

4 participants