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

Avoid conflict between mingw-std-threads and Clang's own #85208

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Nov 22, 2023

Since mingw-std-threads would inject its symbols into std if it thinks there's no other implementation around while using a MinGW compiler (#if (__cplusplus < 201703L) || (defined(__MINGW32__ ) && !defined(_GLIBCXX_HAS_GTHREADS)), it will break LLVM-MinGW builds. The reason is that LLVM-MinGW provides implementations but doesn't define _GLIBCXX_HAS_GTHREADS.

This PR rewrites the patch to change the check to #if defined(__MINGW32__ ) && !(defined(_GLIBCXX_HAS_GTHREADS) && !defined(__clang__)) (#if defined(__MINGW32__ ) && !defined(_GLIBCXX_HAS_GTHREADS) && !defined(__clang__)).

Another option would have been just not to use mingw-std-threads at all on LLVM-MinGW, but I see value in using the same implemention in both MinGW-style compilers. Other opinions welcome.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I agree with using the same mingw-std-threads implementation on all toolchains.

@RandomShaper
Copy link
Member Author

I think I've messed up the condition. Will force-push in a minute.

@akien-mga
Copy link
Member

The new condition makes sense to me, but the patch file seems out of sync.

@RandomShaper RandomShaper force-pushed the fix_mingw_clang branch 2 times, most recently from 99bd090 to 6aea7e1 Compare November 22, 2023 11:19
@RandomShaper
Copy link
Member Author

Please let me know how it looks now. I can't trust my eyes today anymore.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good!

@bitsawer
Copy link
Member

bitsawer commented Nov 22, 2023

Tested with llvm-mingw Clang on Windows and seems to now build and run after some quick test, fixes the build errors I mentioned on RocketChat.

One extra change is needed to fix building with dev_mode enabled. Clang warns about an unused variable as the changes in the patch file now make errnum variable unused. Clang seems to be a bit stricter compared to other compilers. Line 146 in godot.patch can probably be adjusted to remove the line completely to fix it:

int errnum = errno;

@RandomShaper
Copy link
Member Author

@bitsawer Thanks lots for testing! Suggestion applied.

@akien-mga akien-mga merged commit f824a67 into godotengine:master Nov 22, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the fix_mingw_clang branch November 22, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants