Erik de Castro Lopo wrote:> I actually think my change is correct. It should be easier to see if you > look at the github version of the change: > > https://github.com/xiph/flac/commit/e120037f3c67b23fd9eef7ccd04d2df57fa1a9a6#diff-9f048b83ff55071de36263cf0f403b2eL209 >FLAC__NO_SSE_OS was never defined anywhere, so I think that the following changes should be made: defined FLAC__NO_SSE_OS -> 0 !defined FLAC__NO_SSE_OS -> 1 defined FLAC__SSE_OS -> FLAC__SSE_OS !defined FLAC__SSE_OS -> !FLAC__SSE_OS So the code #if defined FLAC__NO_SSE_OS /* assume user knows better than us; turn it off */ disable_sse(info); #elif defined FLAC__SSE_OS /* assume user knows better than us; leave as detected above */ #elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) ... becomes #if 0 /* assume user knows better than us; turn it off */ disable_sse(info); #elif FLAC__SSE_OS /* assume user knows better than us; leave as detected above */ #elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) ... which is equivalent to #if FLAC__SSE_OS /* assume user knows better than us; leave as detected above */ #elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) ... (and there should be no "&& !FLAC__SSE_OS" after "#elif defined(__linux__)")> But yes, all those #ifs and #elses are horrible for readability and > maintainability.OTOH dead code elimination (as in ffmpeg) makes it impossible to build debug version... Probably it's better to have one main simple cpu.h and several additional files (one per architecture) but I'm not going to volunteer for the refactoring.> CHeers, > ErikP.S. I wonder what's the point to test OS SSE support in 2016? I believe that users of Windows 95 and Windows NT4 don't expect new software to work on their OSes. Don't know about users of ancient versions of Linux/BSD/etc though.
lvqcl wrote:> Erik de Castro Lopo wrote: > >> I actually think my change is correct. It should be easier to see if you >> look at the github version of the change: >> >> https://github.com/xiph/flac/commit/e120037f3c67b23fd9eef7ccd04d2df57fa1a9a6#diff-9f048b83ff55071de36263cf0f403b2eL209 >>P.P.S. 1) if FLAC__SSE_OS==1 then MSVC cannot compile current cpu.c: "#include <windows.h>" line is inactive and MSVC complains: cpu.c(278): error C2065: 'EXCEPTION_EXECUTE_HANDLER': undeclared identifier cpu.c(279): error C2065: 'STATUS_ILLEGAL_INSTRUCTION': undeclared identifier 2) the current code (simplified): if(info->ia32.sse) { #if !FLAC__SSE_OS /* assume user knows better than us; turn it off */ disable_sse(info); #elif ... #elif defined(__linux__) && !FLAC__SSE_OS #elif ... #else #endif } means that the __linux__ part is never compiled: if FLAC__SSE_OS==0 then this code becomes just "disable_sse(info);" and if FLAC__SSE_OS==1 then "defined(__linux__) && !FLAC__SSE_OS" is always false.
lvqcl wrote:> 1) if FLAC__SSE_OS==1 then MSVC cannot compile current cpu.c: > "#include <windows.h>" line is inactive and MSVC complains: > cpu.c(278): error C2065: 'EXCEPTION_EXECUTE_HANDLER': undeclared identifier > cpu.c(279): error C2065: 'STATUS_ILLEGAL_INSTRUCTION': undeclared identifier > > 2) the current code (simplified): > > if(info->ia32.sse) { > #if !FLAC__SSE_OS > /* assume user knows better than us; turn it off */ > disable_sse(info); > #elif ... > #elif defined(__linux__) && !FLAC__SSE_OS > #elif ... > #else > #endif > } > > means that the __linux__ part is never compiled: if FLAC__SSE_OS==0 then > this code becomes just "disable_sse(info);" and if FLAC__SSE_OS==1 > then "defined(__linux__) && !FLAC__SSE_OS" is always false.I think I've fixed both those in: commit 23778a3a6018f5dcb5fc1ad6ac97ad8391afc69d Author: Erik de Castro Lopo <erikd at mega-nerd.com> Date: Sat Jun 25 17:02:06 2016 +1000 libFLAC/cpu.c: More pre-processor cleanups I've tested on this in x86, x86_64, powerpc and armhf linux as well as cross-compiling from linux to x86 and x86_64 Windows. Please test compiling on Windows and anything else you can get your hands on. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/