[libcxx-commits] [PATCH] D59525: Speed up certain locale functions on Windows

[Tom Anderson via Phabricator via libcxx-commits]

thomasanderson marked an inline comment as done.
thomasanderson added inline comments.


================
Comment at: include/__locale:67
         __status(_configthreadlocale(_ENABLE_PER_THREAD_LOCALE)),
-        __locale_collate(setlocale(LC_COLLATE, __l.__get_locale())),
-        __locale_ctype(setlocale(LC_CTYPE, __l.__get_locale())),
-        __locale_monetary(setlocale(LC_MONETARY, __l.__get_locale())),
-        __locale_numeric(setlocale(LC_NUMERIC, __l.__get_locale())),
-        __locale_time(setlocale(LC_TIME, __l.__get_locale()))
-        // LC_MESSAGES is not supported on Windows.
+        __locale_all(setlocale(LC_ALL, __l.__get_locale()))
     {}
----------------
smeenai wrote:
> I'm not really familiar with locales, but if I'm reading [MSDN](https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setlocale-wsetlocale?view=vs-2017) correctly, wouldn't you need a call with a NULL `locale` argument to get the value of all categories? I'm not sure how you'd then use that to restore the categories though.
> 
> Also, "Global or thread local storage is used for the string returned by setlocale. Later calls to setlocale overwrite the string, which invalidates string pointers returned by earlier calls." Shouldn't we be copying the return value instead of just storing a pointer? (This is already potentially a problem with the existing code, unless I'm misunderstanding things.)
Yeah, you're right.  How was this ever working? haha

I've reverted the __locale changes.  I'll try to write a fix in a separate CL.  For now, the change in locale_win32.cpp is all that's needed to fix Chromium's perf issues after switching to libc++ on Windows.


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

https://reviews.llvm.org/D59525