[libcxx-commits] [PATCH] D69554: [libcxx] [Windows] Make a more proper implementation of strftime_l for mingw with msvcrt.dll

Sat Nov 2 13:32:51 PDT 2019

mstorsjo added a comment.

In D69554#1731282 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69554/new/

https://reviews.llvm.org/D69554