This is an archive of the discontinued LLVM Phabricator instance.

Fix and speedup __libcpp_locale_guard on Windows
ClosedPublic

Authored by thomasanderson on Mar 19 2019, 6:17 PM.

Details

Summary

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.

Diff Detail

Event Timeline

thomasanderson created this revision.Mar 19 2019, 6:17 PM

+ @smeenai please review

@EricWF or @ldionne or @mclow.lists please review and give formal approval

Handle memory alloc failures

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.

What happens here when the user has different locales for different categories?

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.

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++.

This revision is now accepted and ready to land.Mar 26 2019, 12:37 PM
smeenai added inline comments.Mar 26 2019, 12:49 PM
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

thomasanderson marked an inline comment as done.Mar 26 2019, 12:54 PM
thomasanderson added inline comments.
include/__locale
85

Yes, I've verified that it does have the correct behavior

smeenai added inline comments.Mar 26 2019, 12:59 PM
include/__locale
85

Awesome, LGTM. Might be worth adding a comment to that effect, since the documentation isn't explicit about it.

Add comment suggested by smeenai@

mclow.lists added inline comments.Mar 26 2019, 2:52 PM
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.

thomasanderson added inline comments.Mar 26 2019, 2:58 PM
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)

mclow.lists added inline comments.Mar 26 2019, 7:17 PM
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.
__throw_bad_alloc, on the other hand, will work fine.

thomasanderson marked an inline comment as done.Mar 27 2019, 11:05 AM
thomasanderson added inline comments.
include/__locale
96

Ok cool, I'll land this as-is in that case

This revision was automatically updated to reflect the committed changes.