[libcxx-commits] [PATCH] D59525: Speed up certain locale functions on Windows
Tom Anderson via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 19 13:04:41 PDT 2019
thomasanderson marked an inline comment as done.
thomasanderson added inline comments.
Comment at: include/__locale:67
- __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()))
> 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
More information about the libcxx-commits