-
Notifications
You must be signed in to change notification settings - Fork 285
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
On WIN32, fix compile errors on Clang with MSVC headers #718
base: master
Are you sure you want to change the base?
Conversation
src/libFLAC/CMakeLists.txt
Outdated
@@ -21,6 +21,9 @@ if(FLAC__CPU_X86_64 OR FLAC__CPU_IA32) | |||
if(WITH_AVX AND MSVC) | |||
set_source_files_properties(fixed_intrin_avx2.c lpc_intrin_avx2.c stream_encoder_intrin_avx2.c lpc_intrin_fma.c PROPERTIES COMPILE_FLAGS /arch:AVX2) | |||
endif() | |||
if(WITH_AVX AND WIN32 AND NOT MSVC) | |||
set_source_files_properties(fixed_intrin_avx2.c lpc_intrin_avx2.c stream_encoder_intrin_avx2.c lpc_intrin_fma.c PROPERTIES COMPILE_FLAGS -mavx2) |
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 don't understand why this is necessary. I regularly build with Clang on Win32 through MinGW, and that works perfectly fine without this patch.
So, either the version of Clang you're using is modified in some way, or there is something with the MSVC header files happening here.
In any case, the if()
is too broad because it also applies to MinGW.
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.
This still doesn't make any sense to me. Why would this only be necessary on Clang on Windows with MSVC headers? I'd rather understand this first. It seems like some logic is missing in https://github.com/xiph/flac/blob/master/src/libFLAC/include/private/cpu.h
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.
Apparently this is a bug in ClangCL: llvm/llvm-project#53520
@@ -221,7 +221,7 @@ static char *posixly_correct; | |||
/* Avoid depending on library functions or files | |||
whose names are inconsistent. */ | |||
|
|||
#ifndef getenv | |||
#if !defined(getenv) && !(defined(__clang__) && defined(_MSC_VER)) |
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'm not too sure what this does, and why it doesn't work for your particular configuration. Maybe it can be removed completely? I think it catches some corner case that existed 30 years ago or something.
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.
It seems on Windows, using Clang with MSVC headers would cause error: conflicting types for 'getenv'
X:\path\to\clang.exe -DHAVE_CONFIG_H -D_DARWIN_C_SOURCE -D_FORTIFY_SOURCE=2 -D_POSIX_PTHREAD_SEMANTICS -D_TANDEM_SOURCE -D__STDC_WANT_IEC_60559_BFP_EXT__ -D__STDC_WANT_IEC_60559_DFP_EXT__ -D__STDC_WANT_IEC_60559_FUNCS_EXT__ -D__STDC_WANT_IEC_60559_TYPES_EXT__ -D__STDC_WANT_LIB_EXT2__ -D__STDC_WANT_MATH_SPEC_FUNCS__ -IX:/path/to/flac/include -IX:/path/to/flac/build -Wall -Wextra -Wstrict-prototypes -Wmissing-prototypes -Waggregate-return -Wcast-align -Wnested-externs -Wshadow -Wundef -Wmissing-declarations -Winline -DNDEBUG -O3 -DNDEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -Wdeclaration-after-statement -fstack-protector-strong -MD -MT src/share/getopt/CMakeFiles/getopt.dir/getopt.c.obj -MF src\share\getopt\CMakeFiles\getopt.dir\getopt.c.obj.d -o src/share/getopt/CMakeFiles/getopt.dir/getopt.c.obj -c X:/path/to/flac/src/share/getopt/getopt.c
X:/path/to/flac/src/share/getopt/getopt.c:225:14: error: conflicting types for 'getenv'
225 | extern char *getenv (const char * name);
| ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\ucrt\stdlib.h:1184:28: note: previous declaration is here
1184 | _DCRTIMP char* __cdecl getenv(
|
Which is defined as
_Check_return_ _CRT_INSECURE_DEPRECATE(_dupenv_s)
_DCRTIMP char* __cdecl getenv(
_In_z_ char const* _VarName
);
in <stdlib.h>.
I think the best way to handle this error is to remove the extern declaration when it is clang and MSVC.
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 now it is only targeting when using Clang and MSVC headers, for MinGW headers contains unistd.h but MSVC ones does not. Adding cmake_policy is to suppress the warnings.
#658