The old implementation assumed the POSIX setlocale() API where the old
locale is returned. On Windows, the _new_ locale is returned. This meant
that __libcpp_locale_guard wasn't resetting the locale on destruction.
The new implementation fixes the above issue and takes advantage of
setlocale(LC_ALL) to reduce the number of calls, and also avoids setting
the locale at all if it's not necessary.
+ @smeenai please review
@EricWF or @ldionne or @mclow.lists please review and give formal approval
What happens here when the user has different locales for different categories?
That said, this feels like a POSIX-ism we're leaking into our Windows implementation. So I'm open to doing whatever is more Windows-like.
Then __setlocale() on line 71 will return a string like "LC_CTYPE=C;LC_MONETARY=en-US;...". This string will then be fed back into __setlocale() on line 85 to restore the categories to what they were.
That said, this feels like a POSIX-ism we're leaking into our Windows implementation. So I'm open to doing whatever is more Windows-like.
Ideally we shouldn't have to use __libcpp_locale_guard at all. I think its only use right now is for internally converting ints (and other numbers) to strings by setting the locale to "C", calling some variant of sprintf("%d"), and restoring the locale. This seems like a 100 lb solution to a 1 lb problem. setlocale() and the printf() family of functions are slow. I think it would be much faster and not very difficult to just implement this in libc++.
| include/__locale | ||
|---|---|---|
| 85 | Have you confirmed that setlocale does the right thing when given the semicolon-separated list of locale settings for the different categories? The docs talk about returning such a string for the setlocale(LC_ALL, nullptr) case, but they don't mention what happens if you try to use such a string to set the locale. https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setlocale-wsetlocale?view=vs-2017 | |
| include/__locale | ||
|---|---|---|
| 85 | Yes, I've verified that it does have the correct behavior | |
| include/__locale | ||
|---|---|---|
| 85 | Awesome, LGTM. Might be worth adding a comment to that effect, since the documentation isn't explicit about it. | |
| include/__locale | ||
|---|---|---|
| 96 | I really doubt that this is going to work for anyone who builds with exceptions disabled. On the other hand, maybe that's "not a thing" on windows and/or mingw. | |
| include/__locale | ||
|---|---|---|
| 96 | Looks like __throw_bad_alloc() calls std::abort() if building without exceptions. (Also there's similar unguarded calls to __throw_bad_alloc() in eg. include/locale) | |
| include/__locale | ||
|---|---|---|
| 96 | This is completely my bad. I was wrong here. I read this as throw bad_alloc, which will not work with exceptions disabled. | |
| include/__locale | ||
|---|---|---|
| 96 | Ok cool, I'll land this as-is in that case | |