[libcxx-commits] [PATCH] D144331: [libc++][format] Implements formatter thread::id.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/thread:234
+template <__fmt_char_type _CharT>
+struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT formatter<__thread_id, _CharT> {
+  public:
----------------



================
Comment at: libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/format.functions.tests.h:17
+template <class CharT, class TestFunction, class ExceptionTest>
+void format_tests(TestFunction check, ExceptionTest check_exception) {
+  // Note the output of std::thread::id is unspecified. The output text is the
----------------
```
std::stringstream s; s << id;
assert(s.str() == std::format("{}", id);
```


================
Comment at: libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/parse.pass.cpp:49
+  std::same_as<typename StringViewT::iterator> auto it = formatter.parse(parse_ctx);
+  assert(it == fmt.end() - (!fmt.empty() && fmt.back() == '}'));
+}
----------------
This seems a bit clever to me. Can we pass the expected end position as a parameter to the function explicitly? So this would look like `it == fmt.begin() + offset`. If you have this pattern in other places, I'm OK to land the patch as-is but we could have a simple patch to change all of them.


================
Comment at: libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/stream.pass.cpp:27
 #include "test_macros.h"
 
+template <class CharT>
----------------
While we're at it, can we also test the output stream version on an empty `std::thread::id`?


================
Comment at: libcxx/test/std/thread/thread.threads/thread.thread.class/thread.thread.id/stream.pass.cpp:36-38
+  // This changed the requirement of ostream to have a
+  //   text representation for charT
+  // this definition is used for both streaming and formatting.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144331