[libcxx-commits] [PATCH] D151559: [libc++] implement std::`jthread`

Fri Jun 2 09:19:24 PDT 2023

huixie90 created this revision.
Herald added a project: All.
huixie90 updated this revision to Diff 527830.
huixie90 marked 4 inline comments as done.
huixie90 added a comment.
huixie90 updated this revision to Diff 527869.
huixie90 updated this revision to Diff 527871.
ldionne published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

rebase on splitting <thread>

huixie90 added a comment.

more tests

huixie90 added a comment.

more tests

================
Comment at: libcxx/include/__stop_token/stop_state.h:18
+#include <__threading_support>
 #include <atomic>

----------------
`#include <__thread/thread.h>`? I think this might require exporting `*` or `std.__threading_support.__thread_id` from the modulemap.

================
Comment at: libcxx/include/__thread/formatter.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Would it be possible to granularize these headers in a separate patch? That should be a quick NFC to review.

================
Comment at: libcxx/include/__thread/jthread.h:10-11
+
+#ifndef _LIBCPP___THREAD_JTHREAD
+#define _LIBCPP___THREAD_JTHREAD
+
----------------

================
Comment at: libcxx/include/__thread/jthread.h:41-42
+  // types
+  using id                 = thread::id;
+  using native_handle_type = thread::native_handle_type;
+
----------------
Note to self: this is tested.

================
Comment at: libcxx/include/__thread/jthread.h:48
+  template <class _Fun, class... _Args>
+  _LIBCPP_HIDE_FROM_ABI explicit jthread(_Fun&& __fun, _Args&&... __args)
+    requires(!std::is_same_v<remove_cvref_t<_Fun>, jthread>)
----------------
Let's make sure we have a test for this note:

> [Note 1: This implies that any exceptions not thrown from the invocation of the copy of f will be thrown in the constructing thread, not the new thread. — end note]

(near https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-5)

Edit: We do already have a test!

================
Comment at: libcxx/include/__thread/jthread.h:71
+  _LIBCPP_HIDE_FROM_ABI jthread& operator=(jthread&& __other) noexcept {
+    if (this != &__other) {
+      if (joinable()) {
----------------
Do you have a test for self-assignment?

Edit: Of course, you do.

================
Comment at: libcxx/include/__thread/jthread.h:132
+
+#endif // _LIBCPP___THREAD_JTHREAD
----------------

================
Comment at: libcxx/include/__thread/this_thread.h:10
+
+#ifndef _LIBCPP___THREAD_THIS_THREAD
+#define _LIBCPP___THREAD_THIS_THREAD
----------------
Same here and elsewhere when you do it in another patch.

================
Comment at: libcxx/test/std/thread/thread.jthread/assign.move.pass.cpp:37
+  }
+
+  // if joinable() is true, calls request_stop() and then join()
----------------
I think we don't have a test for the case where the source thread isn't joinable.

================
Comment at: libcxx/test/std/thread/thread.jthread/cons.func.token.pass.cpp:111
+  {
+    struct Exception {
+      std::jthread::id threadId;
----------------
`TEST_HAS_NO_EXCEPTIONS`

================
Comment at: libcxx/test/std/thread/thread.jthread/cons.func.token.pass.cpp:147
+
+  // TODO: test the following:
+  //
----------------
Per your suggestion, we might be able to test https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-8 by setting multiple threads' affinity to the same core. It might not be possible to test this in a robust way, and I think it would be reasonable not to.

================
Comment at: libcxx/test/std/thread/thread.jthread/dtor.pass.cpp:24
+  // !joinable()
+  { std::jthread jt; }
+
----------------

================
Comment at: libcxx/test/std/thread/thread.jthread/dtor.pass.cpp:30
+    std::optional<std::jthread> jt([] {});
+    bool called = false;
+    std::stop_callback cb(jt->get_stop_token(), [&called] { called = true; });
----------------
`assert(jt.joinable()); // test the test`

Those are just suggestions, I'm actually not sure how much I like this.

================
Comment at: libcxx/test/std/thread/thread.jthread/dtor.pass.cpp:39-45
+    bool called = false;
+    std::optional<std::jthread> jt([&called] {
+      std::this_thread::sleep_for(std::chrono::milliseconds{2});
+      called = true;
+    });
+    jt.reset();
+    assert(called);
----------------
To increase the confidence in this test, we could instead spawn more threads and do the same check. That should get the probability that `join()` isn't called but the test still passes down.

Also, I'd leave a short comment behind explaining this particularity of the test (i.e. it's not going to catch issues 100%).

================
Comment at: libcxx/test/std/thread/thread.jthread/join.pass.cpp:28
+  {
+    std::atomic_bool flag = false;
+    std::jthread jt{[&] {
----------------
Same comment here about non-determinism.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151559

Files:
  libcxx/include/CMakeLists.txt
  libcxx/include/__stop_token/stop_state.h
  libcxx/include/__thread/jthread.h
  libcxx/include/module.modulemap.in
  libcxx/include/thread
  libcxx/test/std/thread/thread.jthread/assign.move.pass.cpp
  libcxx/test/std/thread/thread.jthread/cons.default.pass.cpp
  libcxx/test/std/thread/thread.jthread/cons.func.token.pass.cpp
  libcxx/test/std/thread/thread.jthread/cons.move.pass.cpp
  libcxx/test/std/thread/thread.jthread/copy.delete.compile.pass.cpp
  libcxx/test/std/thread/thread.jthread/dtor.pass.cpp
  libcxx/test/std/thread/thread.jthread/join.pass.cpp
  libcxx/test/std/thread/thread.jthread/joinable.pass.cpp
  libcxx/test/std/thread/thread.jthread/nodiscard.verify.cpp
  libcxx/test/std/thread/thread.jthread/swap.member.pass.cpp
  libcxx/test/std/thread/thread.jthread/typedef.compile.pass.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151559.527871.patch
Type: text/x-patch
Size: 29405 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230602/f044a1ca/attachment-0001.bin>