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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__thread/thread.h:26
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+#  include <ios>
+#endif
----------------
ldionne wrote:
> 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).
Good point, I need locale too. However adding these two headers does not change the transitive includes.


================
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.
+    //
----------------
ldionne wrote:
> I don't understand that sentence. Do you mean that fill, align and width can or can't affect the data in the stream?
They can since they affect text.


================
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
----------------
ldionne wrote:
> Is it ever possible for a thread id to be a negative number? If not, am I right that this isn't required?
I don't know, depends on what the platform does.


================
Comment at: libcxx/include/__thread/thread.h:184
+
+    __os.flags(__default | ((__flags & ios_base::left) ? ios_base::left : ios_base::right));
+    __os << __id.__id_;
----------------
JMazurkiewicz wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153336