[libc++] Fixes thread::id's operator<<.

[Mordante]

Summary

The output of

template<class charT, class traits>
  basic_ostream<charT, traits>&
    operator<<(basic_ostream<charT, traits>& out, thread::id id);

is affected by the state of out. The wording states

[thread.thread.id]/2

The text representation for the character type charT of an object of
type thread::id is an unspecified sequence of charT such that, for two
objects of type thread::id x and y, if x == y is true, the thread::id
objects have the same text representation, and if x != y is true, the
thread::id objects have distinct text representations.

[thread.thread.id]/9

template<class charT, class traits>
  basic_ostream<charT, traits>&
    operator<< (basic_ostream<charT, traits>& out, thread::id id);

Effects: Inserts the text representation for charT of id into out.

This wording changed in C++23 due to adding a formatter specialization for
thread::id. However the requirement was the same in older versions of C++.

This issue is that thread::id is an integral or pointer and affected by the
formatting manipulators for them. Thus the text representation can differ if
x == y which violates the requirements.

The fix has to hard-code some formatting style for the text
representation. It uses the Standard specified default values

Table 124: basic_ios::init() effects [tab:basic.ios.cons] flags()

flags() skipws | dec

Fixes PR: https://llvm.org/PR62073

[Mordante]

CI fixes.

[Mordante]

Fixes C++03 compatibility of the test.

[Mordante]

CI fixes.

[Mordante]

libcxx/include/__thread/thread.h

133–158

@ldionne This fails with localization disabled. To old operator<< would also fail but was not "removed" when localization was disabled. I can put to old code in the #else branch, but I wonder whether that is useful.

Note that we normally not comment out operator<<. Maybe we can discuss whether we should do that. I expect it "works" in our tests since they are disabled and these operators are templated on the _CharT of the basic_ostream thus not instantiated automatically.

[ldionne]

libcxx/include/__thread/thread.h

28

Is there any reason why we need <ios> and not just <iosfwd>? It would be really nice to try to avoid including <ios> from here since that's kind of a heavy header (especially with backward compatibility transitive includes enabled).

133–158

I think you did the right thing in this patch. These operator<< should be #ifdef'd out, and if they are not then it's just an oversight that happens not to trigger any issue. We probably forward declare basic_ostream somewhere and it all "works" as long as you don't actually instantiate the operator<<.

152–153

I don't understand that sentence. Do you mean that fill, align and width can or can't affect the data in the stream?

155–157

168–184

Is it ever possible for a thread id to be a negative number? If not, am I right that this isn't required?

[JMazurkiewicz]

libcxx/include/__thread/thread.h

186

Should we should use C locale while printing? Values from std::numpunct (thousands_sep and grouping) may affect text representation when type of __id.__id_ is integral.

I've mentioned it in this issue too: https://github.com/llvm/llvm-project/issues/62073.

[Mordante]

libcxx/include/__thread/thread.h

28

Good point, I need locale too. However adding these two headers does not change the transitive includes.

152–153

They can since they affect text.

168–184

I don't know, depends on what the platform does.

186

Good point, I overlooked the locale part in the bug report. I already felt the original patch was a bit ugly saving and restoring locale seems even worse. Instead of that approach I now create a new stream in a known state with a known locale and output that stream. This might be a bit more expensive, but I don't expect people not to try to use logging unconditionally in expensive parts of their code.

[Mordante]

Addresses review comments.

[ldionne]

Thanks for fixing this! LGTM w/ comments applied and green CI. Also make sure to rebase cause I made some changes to __thread/.

libcxx/include/__thread/thread.h

137

I think this whole comment could actually be changed to:

// [thread.thread.id]/9
//   Effects: Inserts the text representation for charT of id into out.
//
// [thread.thread.id]/2
//   The text representation for the character type charT of an
//   object of type thread::id is an unspecified sequence of charT
//   such that, for two objects of type thread::id x and y, if
//   x == y is true, the thread::id objects have the same text
//   representation, and if x != y is true, the thread::id objects
//   have distinct text representations.
//
// Since various flags in the output stream can affect how the
// thread id is represented (e.g. numpunct or showbase), we
// use a temporary stream instead and just output the thread
// id representation as a string.

161

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/stream.pass.cpp

84–85

[Mordante]

Rebased and addresses review comments.

[Mordante]

In D153336#4490426, @ldionne wrote:

Thanks for fixing this! LGTM w/ comments applied and green CI. Also make sure to rebase cause I made some changes to __thread/.

The rebase when smooth not git issues, however is it intended that thread::id's operator<< is moved to the id.h header?

[Mordante]

CI fixes.

[Mordante]

libcxx/include/__thread/thread.h

28

@ldionne it seems there were additional includes needed after all.

libcxx/include/__thread/thread.h

 #include <__mutex/mutex.h>
 #include <__system_error/system_error.h>
 #include <__thread/id.h>
 #include <__threading_support>
 #include <__utility/forward.h>
 #include <iosfwd>
 #include <tuple>
 
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+#  include <locale>
+#endif
+
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp> class __thread_specific_ptr;
 class _LIBCPP_EXPORTED_FROM_ABI __thread_struct;

 {
     _LIBCPP_INLINE_VISIBILITY
     size_t operator()(__thread_id __v) const _NOEXCEPT
     {
         return hash<__libcpp_thread_id>()(__v.__id_);
     }
 };
 
-template<class _CharT, class _Traits>
-_LIBCPP_INLINE_VISIBILITY
-basic_ostream<_CharT, _Traits>&
-operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id)
-{return __os << __id.__id_;}
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+template <class _CharT, class _Traits>
+_LIBCPP_INLINE_VISIBILITY basic_ostream<_CharT, _Traits>&
+operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id) {
+    // [thread.thread.id]/9
+    //   Effects: Inserts the text representation for charT of id into out.
+    //
+    // [thread.thread.id]/2
+    //   The text representation for the character type charT of an
+    //   object of type thread::id is an unspecified sequence of charT
+    //   such that, for two objects of type thread::id x and y, if
+    //   x == y is true, the thread::id objects have the same text
+    //   representation, and if x != y is true, the thread::id objects
+    //   have distinct text representations.
+    //
+    // Since various flags in the output stream can affect how the
+    // thread id is represented (e.g. numpunct or showbase), we
+    // use a temporary stream instead and just output the thread
+    // id representation as a string.
+
+    basic_ostringstream<_CharT, _Traits> __sstr;
+    __sstr.imbue(locale::classic());
+    __sstr << __id.__id_;
+    return __os << __sstr.str();
+}
+#endif // _LIBCPP_HAS_NO_LOCALIZATION
 
 class _LIBCPP_EXPORTED_FROM_ABI thread
 {
     __libcpp_thread_t __t_;
 
     thread(const thread&);
     thread& operator=(const thread&);
 public:
     typedef __thread_id id;
     typedef __libcpp_thread_t native_handle_type;
 
     _LIBCPP_INLINE_VISIBILITY
     thread() _NOEXCEPT : __t_(_LIBCPP_NULL_THREAD) {}
 #ifndef _LIBCPP_CXX03_LANG
     template <class _Fp, class ..._Args,
               class = __enable_if_t<!is_same<__remove_cvref_t<_Fp>, thread>::value> >
         _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
         explicit thread(_Fp&& __f, _Args&&... __args);
 #else  // _LIBCPP_CXX03_LANG
     template <class _Fp>
     _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
     explicit thread(_Fp __f);
 #endif
     ~thread();
 
     _LIBCPP_INLINE_VISIBILITY
     thread(thread&& __t) _NOEXCEPT : __t_(__t.__t_) {
         __t.__t_ = _LIBCPP_NULL_THREAD;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     thread& operator=(thread&& __t) _NOEXCEPT {
         if (!__libcpp_thread_isnull(&__t_))
             terminate();
         __t_ = __t.__t_;
         __t.__t_ = _LIBCPP_NULL_THREAD;

libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/stream.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: no-threads
 // UNSUPPORTED: no-localization
 // UNSUPPORTED: GCC-ALWAYS_INLINE-FIXME
 
+// REQUIRES: locale.fr_FR.UTF-8
+
 // TODO FMT This test should not require std::to_chars(floating-point)
 // XFAIL: availability-fp_to_chars-missing
 
 // <thread>
 
 // class thread::id
 
 // template<class charT, class traits>
 // basic_ostream<charT, traits>&
 // operator<<(basic_ostream<charT, traits>& out, thread::id id);
 
-#include <thread>
+#include <cassert>
 #include <format>
+#include <locale>
 #include <sstream>
-#include <cassert>
+#include <thread>
 
 #include "make_string.h"
+#include "platform_support.h" // locale name macros
 #include "test_macros.h"
 
 template <class CharT>
-static void test() {
+static void basic() {
   std::thread::id id0 = std::this_thread::get_id();
   std::basic_ostringstream<CharT> os;
   os << id0;
 
 #if TEST_STD_VER > 20
   // C++23 added a formatter specialization for thread::id.
   // This changed the requirement of ostream to have a
   // [thread.thread.id]/2
   //   The text representation for the character type charT of an object of
   //   type thread::id is an unspecified sequence of charT ...
   // This definition is used for both streaming and formatting.
   //
   // Test whether the output is identical.
   std::basic_string<CharT> s = std::format(MAKE_STRING_VIEW(CharT, "{}"), id0);
   assert(s == os.str());
 #endif
 }
 
+template <class CharT>
+static std::basic_string<CharT> format(std::ios_base::fmtflags flags) {
+  std::basic_stringstream<CharT> sstr;
+  sstr.flags(flags);
+  sstr << std::this_thread::get_id();
+  return sstr.str();
+}
+
+template <class CharT>
+static void stream_state() {
+  std::basic_stringstream<CharT> sstr;
+  sstr << std::this_thread::get_id();
+  std::basic_string<CharT> expected = sstr.str();
+
+  // Unaffected by fill, width, and align.
+
+  assert(expected == format<CharT>(std::ios_base::dec | std::ios_base::skipws)); // default flags
+
+  assert(expected == format<CharT>(std::ios_base::oct));
+  assert(expected == format<CharT>(std::ios_base::hex));
+
+  assert(expected == format<CharT>(std::ios_base::scientific));
+  assert(expected == format<CharT>(std::ios_base::fixed));
+
+  assert(expected == format<CharT>(std::ios_base::boolalpha));
+  assert(expected == format<CharT>(std::ios_base::showbase));
+  assert(expected == format<CharT>(std::ios_base::showpoint));
+  assert(expected == format<CharT>(std::ios_base::showpos));
+  assert(expected == format<CharT>(std::ios_base::skipws));  // added for completeness
+  assert(expected == format<CharT>(std::ios_base::unitbuf)); // added for completeness
+  assert(expected == format<CharT>(std::ios_base::uppercase));
+
+  // Test fill, width, and align.
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size() + 10); // Make sure fill and align affect the output.
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::right);
+  sstr << std::this_thread::get_id();
+  expected = sstr.str();
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('*'));
+  sstr.width(expected.size());
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::right);
+  sstr << std::this_thread::get_id();
+  assert(expected != sstr.str());
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size() - 1);
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::right);
+  sstr << std::this_thread::get_id();
+  assert(expected != sstr.str());
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size());
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::left);
+  sstr << std::this_thread::get_id();
+  assert(expected != sstr.str());
+
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size());
+  sstr.flags(std::ios_base::dec | std::ios_base::skipws | std::ios_base::internal);
+  sstr << std::this_thread::get_id();
+  assert(expected == sstr.str()); // internal does *not* affect strings
+
+  // Test the locale's numpunct.
+
+  std::locale::global(std::locale(LOCALE_fr_FR_UTF_8));
+  sstr.str(std::basic_string<CharT>());
+  sstr.fill(CharT('#'));
+  sstr.width(expected.size());
+  sstr << std::this_thread::get_id();
+  assert(expected == sstr.str());
+}
+
+template <class CharT>
+static void test() {
+  basic<CharT>();
+  stream_state<CharT>();
+}
+
 int main(int, char**) {
   test<char>();
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
   test<wchar_t>();
 #endif
 
   return 0;
 }