Pali Rohár <[email protected]> wrote: > Hello, I checked these patches. For me it looks good, I have just few > comments. > > "[PATCH 05/13] crt: update logic in _configthreadlocale stub" > Maybe switch statement would be better to describe all possible input > argument?
This may be a good idea. > "[PATCH 08/13] crt: locale.h: re-group declarations [3/5]" > "[PATCH 09/13] crt: locale.h: re-group declarations [4/5]" > It looks suspicious if the libmsvcrt-os.a code in files _configthreadlocale.c, > _create_locale.c, _free_locale.c and _get_current_locale.c contains > "#define __MSVCRT_VERSION__ 0x0800". This define should be used > exclusively for compiling libmsvcr80.a, not for libmsvcrt.a (or any other). > I think that it is bad idea to redefine __MSVCRT_VERSION__ during > compilation of mingw-w64. It is really needed? Because it is not > explained in commit message why is this change, and my feeling is that > it is not needed. mingw-w64-crt code is being compiled with flag > -D__MSVCRT_VERSION__=0x0600 which is already covered by existing checks > and should provide declarations of all those functions. Original version of the changes was supposed to expose `_configthreadlocale` only for msvcr80.dll and later, but it ended up breaking libc++ builds, so I had to keep it exposed for msvcrt.dll. I think here, "#define __MSVCRT_VERSION__ 0x0800", does make sense. If we ever stop exposing it for msvcrt.dll, there will be no need to change _configthreadlocale.c. Yes, it is here just to expose the declaration. OTOH, it seems that we can remove it for `_create_locale` etc. since we have "-D_WIN32_WINNT=0x0f00" in mingw-w64-crt/Makefile.am, which will always expose them for libmsvcrt.a compilation. So, it would work even with the original guard, which was supposed to be ``` #if __MSVCRT_VERSION__ >= 0x800 || (__MSVCRT_VERSION__ == 0x600 && _WIN32_WINNT >= 0x602) ``` which would expose them for - msvcr80.dll or later - msvcrt.dll for Windows 8 and later Similarly to `_configthreadlocale`, original change breaks libc++ builds with msvcrt.dll, so the guard had to be relaxed. > Note that we have _CRTBLD macro which is already used in mingw-w64-headers > to hide some symbols/functions. So we can use it also for defining > symbols needed for mingw-w64-crt compilation (if needed). I would like to avoid using it if possible, that would keep preprocessor conditions simpler and more readable. But if you think it is better than redefining `__MSVCRT_VERSION__`, I can change the guard to use it. - Kirill Makurin _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
