[libcxx-commits] [PATCH] D119441: [libc++] Fix locale name construction

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch! `locale`s are not my strength, so I left some comment but TBH I don't fully grok why we'd want to make this change. Are we non-conforming at the moment?



================
Comment at: libcxx/src/locale.cpp:325-327
       name_("*")
 {
+  name_ = build_name(other.name_, name, c);
----------------
Ditto below.


================
Comment at: libcxx/src/locale.cpp:326
 {
-    facets_ = other.facets_;
-    for (unsigned i = 0; i < facets_.size(); ++i)
----------------
You seem to have gone from 4 spaces to 2 spaces in the indentation? Please indent consistently with the rest of the code.


================
Comment at: libcxx/src/locale.cpp:558
 
+string locale::__imp::build_name(string other, string one, locale::category c) {
+  if (c == locale::all)
----------------
The Standard says for e.g. `locale( const locale& other, const locale& one, category cat );`:

> Constructs a copy of other except for all the facets identified by the cat argument, which are copied from one. If both other and one have names, then the resulting locale also has a name.

Here, it seems that if `other` doesn't have a name but `one` does, the resulting locale will have a name. Is that intended?



================
Comment at: libcxx/src/locale.cpp:567
+    return "*";
+
+  // Find out user preferred locale
----------------
This is nitpicky, but this should be indented 4 spaces to be consistent with surrounding code.


================
Comment at: libcxx/test/std/localization/locales/locale/locale.cons/name_construction.pass.cpp:24-26
+  std::locale loc1(LOCALE_en_US_UTF_8);
+  std::locale loc2(LOCALE_fr_FR_UTF_8);
+  std::locale loc3(std::locale(), new std::ctype<char>);
----------------
Perhaps this would be easier to follow below?

```
std::locale us(LOCALE_en_US_UTF_8);
std::locale fr(LOCALE_fr_FR_UTF_8);
std::locale unnamed(std::locale(), new std::ctype<char>);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119441