-
Notifications
You must be signed in to change notification settings - Fork 36
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
WIP: Support using clang in mingw64 environment on Windows #4
base: master
Are you sure you want to change the base?
Conversation
add_definitions(-D__STDC_CONSTANT_MACROS) | ||
|
||
if (MSVC) | ||
add_definitions(/bigobj) |
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.
add_definitions
is outdated and has been replaced with newer commands. Maybe it can be replaced here then, see
https://cmake.org/cmake/help/latest/command/add_definitions.html for more information.
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.
Sure, add_compile_options, but add_definitions is used all over the place. Probably do a mass replace as a separate PR.
Source/cmake/OptionsCommon.cmake
Outdated
@@ -1,6 +1,6 @@ | |||
set(CMAKE_CXX_STANDARD 17) | |||
set(CMAKE_CXX_STANDARD_REQUIRED ON) | |||
set(CMAKE_CXX_EXTENSIONS OFF) | |||
set(CMAKE_CXX_EXTENSIONS ON) |
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.
Any reason this is required? This should probably be off, as it allows compiler specific extensions to the language and can cause confusing errors.
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.
No, I think this was an error
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.
From trying to build again, looks like this wasn't an error.
@@ -130,40 +130,50 @@ endif () | |||
|
|||
# Check whether features.h header exists. | |||
# Including glibc's one defines __GLIBC__, that is used in Platform.h | |||
WEBKIT_CHECK_HAVE_INCLUDE(HAVE_FEATURES_H features.h) | |||
if (NOT (${CMAKE_SYSTEM_NAME} STREQUAL "Windows")) |
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.
Is there a reason this check is not a WIN32
check? (Same goes for the following ones)
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 think CMAKE_SYSTEM_NAME is for target, not for host.
Don't want to target Windows under Linux or Mac and assume these are available.
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.
WIN32
is set to TRUE
when targeting a Windows (like) system, so if you compile with MinGW under Linux, CMake will define WIN32
.
It is also a bit weird that they have to be guarded at all. WEBKIT_CHECK_HAVE_INCLUDE
checks if the include exists, and if it doesn't, it does not fail the compilation, but just sets the first argument of the macro to false. So even if those checks would be executed on every system, it should under no circumstances break the build.
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 case I want to avoid is if they return true under Windows, not if they return false.
Primary goal:
Build and run JSC built with Clang under windows
Build and test JSC built with Clang and thin-LTO successfully under windows
Other stuff:
Support building without Udis86 / Disassembler
Support building release with (some) asserts
Use UASM instead of MASM
Drop UASM and MASM mostly or entirely if possible
Fix CPU guessing under Clang on Windows
Support building directly from sources instead of "unified sources"
Current CMake magic required to build, to be integrated into CMakeLists or other:
TODO: Look into using CMAKE_SYSROOT, CMAKE_TOOLCHAIN_FILE
This is using Windows CMake instead of MSys/MinGW64 Cmake.
Also needs;