Support for non-CL Clang on Windows #981
Conversation
Clang on Windows defines _MSC_VER, but it doesn't understand _Printf_format_string_
It doesn't generate separate Debug and Release directories and instead puts the executable directly under build/
There was a problem hiding this comment.
Mostly LGTM but I believe @saghul reviewed this the first time around so he's likely more qualified to sign off on it.
CMakeLists.txt
Outdated
| if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32) | ||
| add_compile_definitions(_CRT_SECURE_NO_WARNINGS) | ||
| add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE) | ||
| endif() |
There was a problem hiding this comment.
| if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32) | |
| add_compile_definitions(_CRT_SECURE_NO_WARNINGS) | |
| add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE) | |
| endif() | |
| if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32) | |
| add_compile_definitions(_CRT_SECURE_NO_WARNINGS) | |
| add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE) | |
| endif() |
| @@ -198,10 +198,10 @@ static void js_set_thread_state(JSRuntime *rt, JSThreadState *ts) | |||
| js_std_cmd(/*SetOpaque*/1, rt, ts); | |||
| } | |||
|
|
|||
| #ifdef __GNUC__ | |||
| #if defined(__GNUC__) || defined(__clang__) | |||
There was a problem hiding this comment.
Doesn't clang-on-windows define __GNUC__? Because clang-on-linux definitely does:
$ clang -E -dM - < /dev/null | grep GNUC
#define __GNUC_MINOR__ 2
#define __GNUC_PATCHLEVEL__ 1
#define __GNUC_STDC_INLINE__ 1
#define __GNUC__ 4
There was a problem hiding this comment.
I believe it does too. What are you trying to fix here @HJfod ?
There was a problem hiding this comment.
Removing the defined(__clang__) check makes it not work, so seems like Clang doesn't define __GNUC__
.github/workflows/ci.yml
Outdated
| @@ -234,7 +234,40 @@ jobs: | |||
| cl.exe /DJS_NAN_BOXING=0 /Zs cxxtest.cc | |||
| cl.exe /DJS_NAN_BOXING=1 /Zs cxxtest.cc | |||
|
|
|||
| windows-clang: | |||
| windows-ninja-clang: | |||
There was a problem hiding this comment.
Nit, can we call this windows-real-clang perhaps?
CMakeLists.txt
Outdated
| @@ -42,6 +42,13 @@ xcheck_add_c_compiler_flag(-Wno-stringop-truncation) | |||
| xcheck_add_c_compiler_flag(-Wno-array-bounds) | |||
| xcheck_add_c_compiler_flag(-funsigned-char) | |||
|
|
|||
| # Clang on Windows without MSVC command line fails because the codebase uses | |||
| # functions like strcpy over strcpy_s | |||
| if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32) | |||
There was a problem hiding this comment.
Don't you also want to add NOT MSVC here, so make sure it's not ClangCL ?
CMakeLists.txt
Outdated
| @@ -77,7 +84,7 @@ endif() | |||
| if(WIN32) | |||
| if(MSVC) | |||
| set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /STACK:8388608") | |||
| else() | |||
| elseif(MINGW) | |||
There was a problem hiding this comment.
Why do you want to skip this on Clang?
There was a problem hiding this comment.
Clang didn't seem to understand the parameter syntax and I couldn't find an equivalent option. Can take another look 👍
| @@ -198,10 +198,10 @@ static void js_set_thread_state(JSRuntime *rt, JSThreadState *ts) | |||
| js_std_cmd(/*SetOpaque*/1, rt, ts); | |||
| } | |||
|
|
|||
| #ifdef __GNUC__ | |||
| #if defined(__GNUC__) || defined(__clang__) | |||
There was a problem hiding this comment.
I believe it does too. What are you trying to fix here @HJfod ?
getopt_compat.h
Outdated
| @@ -68,6 +68,8 @@ | |||
| #include <stdarg.h> | |||
| #include <stdio.h> | |||
| #include <windows.h> | |||
| /* Required for JS_PRINTF_FORMAT */ | |||
There was a problem hiding this comment.
Can we not do this and silence the warnings if need be? I actually want to get rid of this header altogether by parsing command line arguments by hand in qjsc, since it's only used there...
* CI action renamed to just `windows-clang` * Remove inclusion of `cutils.h` from `getopt_compat.h` and just suppress format string literal warnings * Add more comments to `quickjs-libc.c` and `quickjs.h` * Update `CMakeLists.txt` to also set 8 MB stack size for Windows Clang
Clang on Windows without MSVC command line is a little bit quirky, so this PR adds support for it. I had an earlier PR (#886) for this too, but closed that one as I got too busy at the time to finish it. Now this works for the whole codebase as well as has CI tests for the new architechture combination.