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

[Hubert Tong via Phabricator via libcxx-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: libcxx/src/locale.cpp:558
 
+string locale::__imp::build_name(string other, string one, locale::category c) {
+  if (c == locale::all)
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > ldionne wrote:
> > > 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?
> > > 
> > Sounds like the specification issue referred to by https://cplusplus.github.io/LWG/lwg-active.html#2295.
> > Sounds like the specification issue referred to by https://cplusplus.github.io/LWG/lwg-active.html#2295.
> 
> Interestingly enough, libc++ chooses to keep the name for the `nullptr` facet case. So, it seems more consistent for libc++ to prefer to keep the name in the `locale::none` case.
> Here, it seems that if `other` doesn't have a name but `one` does, the resulting locale will have a name. Is that intended?

Oh, you are talking of the `locale::all` case more than the `locale::none` one. I agree that the `locale::all` case is a problem. For example, the standard categories do not necessarily cover all of the categories present in the C implementation. `locale:all` should only modify the standard categories in terms of the name.


================
Comment at: libcxx/src/locale.cpp:559-560
+string locale::__imp::build_name(string other, string one, locale::category c) {
+  if (c == locale::all)
+    return one;
+  if (c == locale::none)
----------------
See above comment.


================
Comment at: libcxx/src/locale.cpp:570
+  if (one == "")
+    one = setlocale(LC_ALL, "");
+
----------------
This call to `setlocale` is not okay. It modifies the current C locale for the program.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119441