[libcxx-commits] [libcxx] [libc++][windows] Use _wsetlocale() in __locale_guard (PR #160479)

[Martin Storsjö via libcxx-commits]

================
@@ -45,7 +45,11 @@ struct __libcpp_locale_guard {
     // each category.  In the second case, we know at least one category won't
     // be what we want, so we only have to check the first case.
     if (std::strcmp(__l.__get_locale(), __lc) != 0) {
-      __locale_all = _strdup(__lc);
+      // Use wsetlocale to query the current locale string. This avoids a lossy
+      // conversion of the locale string from UTF-16 to the current LC_CTYPE
+      // charset. The Windows CRT allows language / country strings outside of
+      // ASCII, e.g. "Norwegian Bokm\u00E5l_Norway.utf8".
+      __locale_all = _wcsdup(__wsetlocale(nullptr));
----------------
mstorsjo wrote:

It's a bit of a pity that this requires calling `__wsetlocale()` a second time after the first `__setlocale()` above; I'm wondering if this is a risk for performance degradation.

See e.g. https://github.com/llvm/llvm-project/issues/56202 for a case where this has been measured to be a bottleneck - CC @alvinhochun.

Here, I guess the alternative is to unconditionally use `__wsetlocale()` above for fetching the name of the current locale, and that requires us to do more of the potentially messy charset conversions. Likewise - the form that this patch suggests feels a bit asymmetrical, when both narrow and wide APIs are being used for the same thing. But I see that it would require more of a mess and more local charset conversions (and require us to decide which charset to use for conversions) if we'd switch over entirely.

So all in all, this is probably fine in this form; I think I agree that this is a reasonable compromise form.

https://github.com/llvm/llvm-project/pull/160479