On Monday 13 July 2026 10:01:52 Kirill Makurin wrote: > 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.
And what is the problem of exporting fallback _configthreadlocale function also for pre-msvcr80 libraries? > I think here, "#define __MSVCRT_VERSION__ 0x0800", does make sense. #define __MSVCRT_VERSION__ 0x0800 says that code is being compiled for msvcrt80.dll. Some header files also adjust some structures which for different msvcr* versions based on __MSVCRT_VERSION__, so it really should be avoided. > 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 Yes, I think it is not needed and can be removed. That could simplify it. > > ``` > #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. Adding those guards more and more stricter can break more applications and builds. Is not it rather better to make guards less strict so they would not have potential of breaking builds & applications? > > 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 Well, the purpose of _CRTBLD is exactly that code is being compiled for mingw-w64 crt runtime. Using one macro (__MSVCRT_VERSION__) for two different things is bad idea. _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
