-
-
Notifications
You must be signed in to change notification settings - Fork 859
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
[Fix CMake for Clang users on Windows] #311
Conversation
Well, MSVC is not the only compiler on Windows, we use also `Clang` that use the same compiler flags as `MSVC` (clang-cl)
Hello! I'm not sure that |
@Innokentiy-Alaytsev he updated it. Does it work as expected now? |
Just a typo. |
Yes, it works fine. Sorry, if I looked into a wrong commit. |
Don't worry. Thank you for trying it. 👍 |
# it seems that -O3 ruins a bit the performance when using clang ... | ||
INTERFACE $<$<AND:$<CONFIG:Release>,$<CXX_COMPILER_ID:Clang>>:-O2> | ||
INTERFACE $<$<AND:$<CONFIG:Release>,$<CXX_COMPILER_ID:Clang>,$<PLATFORM_ID:Darwin>>:-O2> |
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.
We can merge this line and the one below as:
INTERFACE $<$<AND:$<CONFIG:Release>,$<CXX_COMPILER_ID:Clang>,$<OR:$<PLATFORM_ID:Darwin>,$<PLATFORM_ID:Linux>>>:-O2>
May I ask you to integrate the PR? Otherwise I can merge it on a separate branch and edit the file before to merge it on master
.
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.
If you can edit it on your side is great I used the GitHub web editor !
I'm applying the changes on my side directly.
maybe we can add a tests ta compile with clang on appveyor ?
|
|
Well, MSVC is not the only compiler on Windows, we use also
Clang
that use the same compiler flags asMSVC
(clang-cl)According to that, when you detect clang we also should detect platform.