[libc++] Fix simple cases of locale name construction

[Jake-Egan]

Summary

When using the following constructors:

locale(const locale& other, const char* std_name, category cat);
locale(const locale& other, const string& std_name, category cat);
locale(const locale& other, const locale& one, category cats);

The new locale name is always "*". Locale names formed from parts of two named locales (that is, C++ locales having names) are supposed to have names in turn (see C++20 subclause 28.3.1.1 [locale.general] paragraph 8). This patch fixes the name construction for cases when either of locales are unnamed, when the category is locale::none, and when the two locale names are the same.

[ldionne]

Thanks for the patch! locales 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?

libcxx/src/locale.cpp

309

You seem to have gone from 4 spaces to 2 spaces in the indentation? Please indent consistently with the rest of the code.

318–323

Ditto below.

548

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?

557

This is nitpicky, but this should be indented 4 spaces to be consistent with surrounding code.

libcxx/test/std/localization/locales/locale/locale.cons/name_construction.pass.cpp

25–27

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>);

[hubert.reinterpretcast]

In D119441#3311394, @ldionne wrote:

Thanks for the patch! locales 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?

Yes, because locales formed from parts of two named locales (that is, C++ locales having names) have names in turn (see C++20 subclause 28.3.1.1 [locale.general] paragraph 8). Locales with names are supposed to compare equal if their names compare equal (see C++20 subclause 28.3.1.5 [locale.operators] paragraph 1). If the implementation wants to give named locales that are formed differently the same name (*), then it is required to report that they are equal. It seems an interesting experiment in conforming but unhelpful implementation methods to retain the * and correct the equality comparison for conformance, thus we did not propose that direction for this patch.

#include <locale>
#include <cassert>
#include <stdio.h>
int main(void) {
  std::locale America("en_US.UTF-8");
  std::locale Generic("C");
  fprintf(stderr, "%s\n", America.name().c_str());
  fprintf(stderr, "%s\n", Generic.name().c_str());

  std::locale Financial(Generic, America, std::locale::monetary);
  fprintf(stderr, "%s\n", Financial.name().c_str());

  std::locale Ascii(America, Generic, std::locale::collate | std::locale::ctype);
  fprintf(stderr, "%s\n", Ascii.name().c_str());

  assert((Financial.name() == Ascii.name()) == (Financial == Ascii));
}

Compiler Explorer link: https://godbolt.org/z/vKj9qMMT4

[hubert.reinterpretcast]

libcxx/src/locale.cpp

548

Sounds like the specification issue referred to by https://cplusplus.github.io/LWG/lwg-active.html#2295.

[hubert.reinterpretcast]

libcxx/src/locale.cpp

548

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.

[hubert.reinterpretcast]

In D119441#3316562, @hubert.reinterpretcast wrote:

If the implementation wants to give named locales that are formed differently the same name (*), then it is required to report that they are equal. It seems an interesting experiment in conforming but unhelpful implementation methods to retain the * and correct the equality comparison for conformance, thus we did not propose that direction for this patch.

I also opened a new issue for libc++ locales that compare equal but behave differently: #53797

[hubert.reinterpretcast]

libcxx/src/locale.cpp

548

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.

549–550

See above comment.

560

This call to setlocale is not okay. It modifies the current C locale for the program.

[hubert.reinterpretcast]

libcxx/src/locale.cpp

560

For example, on Linux, the name of the locale can be discovered piece-wise by using _NL_LOCALE_NAME:

locale_t p = newlocale(LC_ALL, "", nullptr);
fprintf(stderr, "%s\n", nl_langinfo_l(_NL_LOCALE_NAME(LC_NUMERIC), p));

574

All signs are that the AIX format is space-separated.

579

584–586

588

Safer to check that the length is not negative because of unexpected format.

591–592

Avoid modifying the string all the time when updating a scan position will do.

598

Although AIX is happy with having 5 here and getting the 6th added after this check, z/OS seems to require 8 (7 at the point of this check).

600

603

627

Same comment as for the previous loop.

630

Same comment as for the previous loop.

637

Should there not be a check that there are at least enough for the indexing based on the category?

677

681–683

These appear to be documented as GNU extensions. The #else block here is unlikely to be appropriate on Darwin, BSD, or Windows.

[hubert.reinterpretcast]

libcxx/src/locale.cpp

559

It is insufficient to only do this for "".
On Linux, the environment is queried for each category that is specified with an empty string in the expanded LC_ALL format, e.g. (https://godbolt.org/z/cvYMcMbx4):

setlocale(LC_ALL, "LC_CTYPE=;LC_NUMERIC=;LC_TIME=;LC_COLLATE=en_US.UTF-8;LC_MONETARY=;LC_MESSAGES=;LC_PAPER=;LC_NAME=;LC_ADDRESS=;LC_TELEPHONE=;LC_MEASUREMENT=;LC_IDENTIFICATION=");

[hubert.reinterpretcast]

For info: getlocalename_l is the proposed/new interface to get (per category) locale names in a thread-safe manner.

[Jake-Egan]

Reducing this patch to the simpler cases for easier review. Will address the more complicated cases in
another patch.

[Jake-Egan]

Fix indent.

[Jake-Egan]

Fix indent 2.

[Jake-Egan]

XFAIL apple backdeployment targets.

[Jake-Egan]

Fix xfail

[Jake-Egan]

Ping

[Mordante]

Can you add the relevant part of your reply to the commit message, that makes looking at the reason for the change easier in the future.

Yes, because locales formed from parts of two named locales (that is, C++ locales having names) have names in turn (see C++20 subclause 28.3.1.1 [locale.general] paragraph 8). Locales with names are supposed to compare equal if their names compare equal (see C++20 subclause 28.3.1.5 [locale.operators] paragraph 1). If the implementation wants to give named locales that are formed differently the same name (*), then it is required to report that they are equal. It seems an interesting experiment in conforming but unhelpful implementation methods to retain the * and correct the equality comparison for conformance, thus we did not propose that direction for this patch.

libcxx/src/locale.cpp

173

Nit why pass by value?
Why a member function instead of a non-member function?

[Jake-Egan]

• Updated the description
• Use pass by reference
• Use non-member function

[Jake-Egan]

Ping

[Jake-Egan]

• Trigger CI

[Mordante]

I see several comments off @ldionne and @hubert.reinterpretcast not marked as done. Are they done?

[Jake-Egan]

In D119441#4340630, @Mordante wrote:

I see several comments off @ldionne and @hubert.reinterpretcast not marked as done. Are they done?

This patch used to be a lot larger, but I've reduced it to a few cases. All the comments regarding the reduced patch have been addressed. I'll be posting another patch that addresses the rest of the comments.

[Jake-Egan]

Ping

[ldionne]

Let's rebase and ship, sorry for the delay. This LGTM now, it's indeed much smaller than initially.

libcxx/src/locale.cpp

125–135

[Jake-Egan]

Thanks @ldionne

[Jake-Egan]

Remove whitespace

libcxx/src/locale.cpp

 #else
     (void)msg;
     _VSTD::abort();
 #endif
 }
 
 }
 
+string
+build_name(const string& other, const string& one, locale::category c) {
+    if (other == "*" || one == "*")
+        return "*";
+    if (c == locale::none || other == one)
+        return other;
+
+    // FIXME: Handle the more complicated cases, such as when the locale has
+    // different names for different categories.
+    return "*";
+}
+
 const locale::category locale::none;
 const locale::category locale::collate;
 const locale::category locale::ctype;
 const locale::category locale::monetary;
 const locale::category locale::numeric;
 const locale::category locale::time;
 const locale::category locale::messages;
 const locale::category locale::all;
public:
 
     static const locale& make_classic();
     static       locale& make_global();
 private:
     void install(facet* f, long id);
     template <class F> void install(F* f) {install(f, f->id.__get());}
     template <class F> void install_from(const __imp& other);
 };
 
 locale::__imp::__imp(size_t refs)
     : facet(refs),
       facets_(N),
       name_("C")
 {
     facets_.clear();
     install(&make<_VSTD::collate<char> >(1u));
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS

 {
     facets_ = other.facets_;
     for (unsigned i = 0; i < facets_.size(); ++i)
         if (facets_[i])
             facets_[i]->__add_shared();
 }
 
 locale::__imp::__imp(const __imp& other, const string& name, locale::category c)
-    : facets_(N),
-      name_("*")
+    : facets_(N), name_(build_name(other.name_, name, c))
 {
     facets_ = other.facets_;
     for (unsigned i = 0; i < facets_.size(); ++i)
         if (facets_[i])
             facets_[i]->__add_shared();
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
     try
     {
 #endif // _LIBCPP_HAS_NO_EXCEPTIONS
         if (c & locale::collate)
         {
             install(new collate_byname<char>(name));
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS

 void
 locale::__imp::install_from(const locale::__imp& one)
 {
     long id = F::id.__get();
     install(const_cast<F*>(static_cast<const F*>(one.use_facet(id))), id);
 }
 
 locale::__imp::__imp(const __imp& other, const __imp& one, locale::category c)
-    : facets_(N),
-      name_("*")
+    : facets_(N), name_(build_name(other.name_, one.name_, c))
 {
     facets_ = other.facets_;
     for (unsigned i = 0; i < facets_.size(); ++i)
         if (facets_[i])
             facets_[i]->__add_shared();
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
     try
     {

 const locale::facet*
 locale::__imp::use_facet(long id) const
 {
     if (!has_facet(id))
         __throw_bad_cast();
     return facets_[static_cast<size_t>(id)];
 }
 
 // locale
 
 const locale&
 locale::__imp::make_classic()
 {
     // only one thread can get in here and it only gets in once
     static aligned_storage<sizeof(locale)>::type buf;
     locale* c = reinterpret_cast<locale*>(&buf);
     c->__locale_ = &make<__imp>(1u);
     return *c;
 }
 
 const locale&
 locale::classic()
 {
     static const locale& c = __imp::make_classic();
     return c;
 }
 
 locale&
 locale::__imp::make_global()
 {
     // only one thread can get in here and it only gets in once
     static aligned_storage<sizeof(locale)>::type buf;
     auto *obj = ::new (&buf) locale(locale::classic());
     return *obj;
 }
 
 locale&
 locale::__global()
 {
     static locale& g = __imp::make_global();
     return g;
 }
 
 locale::locale() noexcept
     : __locale_(__global().__locale_)
 {
     __locale_->__add_shared();
 }
 
 locale::locale(const locale& l) noexcept
     : __locale_(l.__locale_)
 {
     __locale_->__add_shared();
 }
 
 locale::~locale()
 {
     __locale_->__release_shared();
 }
 
 const locale&
 locale::operator=(const locale& other) noexcept
 {
     other.__locale_->__add_shared();
     __locale_->__release_shared();
     __locale_ = other.__locale_;
     return *this;
 }
 
 locale::locale(const char* name)
     : __locale_(name ? new __imp(name)
                      : (__throw_runtime_error("locale constructed with null"), nullptr))
 {
     __locale_->__add_shared();
 }
 
 locale::locale(const string& name)
     : __locale_(new __imp(name))
 {
     __locale_->__add_shared();
 }
 
 locale::locale(const locale& other, const char* name, category c)
     : __locale_(name ? new __imp(*other.__locale_, name, c)
                      : (__throw_runtime_error("locale constructed with null"), nullptr))
 {
     __locale_->__add_shared();
 }
 
 locale::locale(const locale& other, const string& name, category c)
     : __locale_(new __imp(*other.__locale_, name, c))
 {
     __locale_->__add_shared();
 }
 
 locale::locale(const locale& other, const locale& one, category c)
     : __locale_(new __imp(*other.__locale_, *one.__locale_, c))
 {
     __locale_->__add_shared();
 }
 
 string
 locale::name() const
 {
     return __locale_->name();
 }

 locale::has_facet(id& x) const
 {
     return __locale_->has_facet(x.__get());
 }
 
 const locale::facet*
 locale::use_facet(id& x) const
 {
     return __locale_->use_facet(x.__get());
 }
 
 bool
 locale::operator==(const locale& y) const
 {
     return (__locale_ == y.__locale_)
         || (__locale_->name() != "*" && __locale_->name() == y.__locale_->name());
 }
 
 // locale::facet
 
 locale::facet::~facet()
 {
 }

libcxx/test/std/localization/locales/locale/locale.cons/name_construction.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: locale.en_US.UTF-8
+// REQUIRES: locale.zh_CN.UTF-8
+
+// <locale>
+
+// Test locale name construction for the following constructors:
+// locale(const locale& other, const char* std_name, category cat);
+// locale(const locale& other, const string& std_name, category cat);
+// locale(const locale& other, const locale& one, category cats);
+
+// This test exercises the fix for locale name construction (D119441), which
+// isn't in the dylib for some systems.
+// XFAIL: stdlib=apple-libc++ && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
+
+#include <locale>
+#include <cassert>
+#include "platform_support.h" // locale name macros
+
+int main(int, char**) {
+    std::locale en(LOCALE_en_US_UTF_8);
+    std::locale zh(LOCALE_zh_CN_UTF_8);
+    std::locale unnamed(std::locale(), new std::ctype<char>);
+    {
+        std::locale loc(unnamed, en, std::locale::time);
+        assert(loc.name() == "*");
+    }
+    {
+        std::locale loc(en, unnamed, std::locale::none);
+        assert(loc.name() == "*");
+    }
+    {
+        std::locale loc(en, "", std::locale::none);
+        assert(loc.name() == en.name());
+    }
+    {
+        std::locale loc(en, zh, std::locale::none);
+        assert(loc.name() == en.name());
+    }
+    {
+        std::locale loc(en, LOCALE_en_US_UTF_8, std::locale::time);
+        assert(loc.name() == en.name());
+    }
+    {
+        std::locale loc(en, std::string(LOCALE_en_US_UTF_8), std::locale::collate);
+        assert(loc.name() == en.name());
+    }
+  return 0;
+}