[libcxx-commits] [PATCH] D153336: [libc++] Fixes thread::id's operator<<.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__thread/thread.h:26
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+#  include <ios>
+#endif
----------------
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).


================
Comment at: libcxx/include/__thread/thread.h:131
 
-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>
----------------
Mordante wrote:
> @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.
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<<`.


================
Comment at: libcxx/include/__thread/thread.h:150-151
+    // affect the text representation. It does not mention that the data
+    // in the stream must be identical for the same thread. So fill
+    // align and width are unaffected can affect the data in the stream.
+    //
----------------
I don't understand that sentence. Do you mean that fill, align and width can or can't affect the data in the stream?


================
Comment at: libcxx/include/__thread/thread.h:153-155
+    // Note this wording it the wording of C++23, which changed due to
+    // the formatter thread::id, however earlier versions of C++
+    // effectively had the same wording.
----------------



================
Comment at: libcxx/include/__thread/thread.h:166-182
+    // The internal alignment may affect the value when the value is
+    // negative
+    // Table 114: Fill padding [tab:facet.num.put.fill]
+    //   adjustfield == internal and a sign occurs in the representation
+    //   pad after the sign
+    //
+    // For a sequence of char
----------------
Is it ever possible for a thread id to be a negative number? If not, am I right that this isn't required?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153336