[libcxx] [Windows] Make a more proper implementation of strftime_l for mingw with msvcrt.dll

[mstorsjo]

Summary

See D37468 for original case when this specific fallback was added.

[rnk]

lgtm

[mstorsjo]

Adding more libcxx/runtimes maintainers, to get an ack from one of them as well before pushing.

[mclow.lists]

This looks reasonable to me, but I noticed that there's a behavior difference - __libcpp_locale_guard has been added. (previously the loc parameter was ignored).

Shouldn't there be a test for this new behavior?

[mstorsjo]

In D69554#1731282, @mclow.lists wrote:

This looks reasonable to me, but I noticed that there's a behavior difference - __libcpp_locale_guard has been added. (previously the loc parameter was ignored).

Yes, this is the whole point of the change. The windows CRTs lack some _l suffixed functions (that can operate on any locale given as a parameter), but have functions that respect the currently set global locale. The wrappers that implement the _l suffixed functions (both this, and most of the existing ones in locale_win32.cpp) temporarily set the current thread's global locale to the desired one, execute the desired function, and then restore it on return.

Shouldn't there be a test for this new behavior?

Ideally yes, but I don't think any of the code in locale_win32.cpp actually is tested so far. E.g. localeconv_l is pretty badly broken at the moment (see D69505) - and practically speaking, I don't have a setup where I can run the runtimes tests on windows yet (I pretty much exclusively cross compile). But I guess I should work on setting that up anyway...

But regarding tests for this particular case; locales in msvcrt.dll (the old legacy one that mingw setups target by default) are extremely limited anyway (so far I've only gotten the default set "C" and the "" locale for the system-default settings to work, nothing else), and they actually can't be configured per-thread, only globally for the whole process. So this is pretty far out in the land of not really working as one would expect.

But this particular patch mostly is a case where I noticed that the current code was inconsistent (to be blamed on myself 2 years ago) and I'd like to make it more consistent.

[mstorsjo]

In D69554#1731282, @mclow.lists wrote:

Shouldn't there be a test for this new behavior?

I looked into the tests, and I think this codepath should be hit by existing tests in e.g. std/input.output/iostream.format/ext.manip/put_time.pass.cpp. (The test in itself doesn't work exactly as is though as msvcrt.dll, which this patch affects, provides a very limited set of locales, not including the one used by that test.)

@mclow.lists - ok to commit this one?

[mstorsjo]

I'll go ahead and land this one tomorrow, before the branch; it's harmless for other configurations and makes the code more consistent. As the locale support in that crt (msvcrt.dll) is so lacking, I don't see a good way to make a meaningful testcase - the main takeaway of the change is consistency.

libcxx/include/support/win32/locale_win32.h

 #define iswlower_l _iswlower_l
 #define iswalpha_l _iswalpha_l
 #define iswdigit_l _iswdigit_l
 #define iswpunct_l _iswpunct_l
 #define iswxdigit_l _iswxdigit_l
 #define towupper_l _towupper_l
 #define towlower_l _towlower_l
 #if defined(__MINGW32__) && __MSVCRT_VERSION__ < 0x0800
-#define strftime_l( __s, __l, __f, __tm, __loc ) strftime( __s, __l, __f, __tm )
+_LIBCPP_FUNC_VIS size_t strftime_l(char *ret, size_t n, const char *format,
+                                   const struct tm *tm, locale_t loc);
 #else
 #define strftime_l _strftime_l
 #endif
 #define sscanf_l( __s, __l, __f, ...) _sscanf_l( __s, __f, __l, __VA_ARGS__ )
 #define sprintf_l( __s, __l, __f, ... ) _sprintf_l( __s, __f, __l, __VA_ARGS__ )
 #define vsprintf_l( __s, __l, __f, ... ) _vsprintf_l( __s, __f, __l, __VA_ARGS__ )
 #define vsnprintf_l( __s, __n, __l, __f, ... ) _vsnprintf_l( __s, __n, __f, __l, __VA_ARGS__ )
 _LIBCPP_FUNC_VIS int snprintf_l(char *ret, size_t n, locale_t loc, const char *format, ...);

libcxx/src/support/win32/locale_win32.cpp

float strtof_l(const char* nptr, char** endptr, locale_t loc) {
   return strtof(nptr, endptr);
 }
 
 long double strtold_l(const char* nptr, char** endptr, locale_t loc) {
   __libcpp_locale_guard __current(loc);
   return strtold(nptr, endptr);
 }
 #endif
+
+#if defined(__MINGW32__) && __MSVCRT_VERSION__ < 0x0800
+size_t strftime_l(char *ret, size_t n, const char *format, const struct tm *tm,
+                  locale_t loc) {
+  __libcpp_locale_guard __current(loc);
+  return strftime(ret, n, format, tm);
+}
+#endif