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

Fri Sep 15 09:33:57 PDT 2023

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

I don't think any of my comments really require me to take another look. This is really good, thank you!

================
Comment at: libcxx/include/CMakeLists.txt:672
   __support/xlocale/__posix_l_fallback.h
   __support/xlocale/__strtonum_fallback.h
   __system_error/errc.h
----------------
Not attached: let's mention this in `UsingLibcxx.rst` where we have a list of experimental features.

================
Comment at: libcxx/include/__stop_token/stop_state.h:18
+#include <__thread/this_thread.h>
+#include <__thread/thread.h>
 #include <atomic>
----------------
I think you can simply replace this by `__thread/id.h` now that it has been split out of `__thread/thread.h`. This might reduce dependencies a bit.

================
Comment at: libcxx/include/__thread/jthread.h:31-33
+#ifdef _LIBCPP_HAS_NO_THREADS
+#  error "<thread> is not supported since libc++ has been configured without support for threads."
+#endif
----------------
I would remove this since there's already one in `<thread>`. We want to revisit how we're handling missing capabilities like this (in some cases we just omit the contents of headers, in other cases like `_LIBCPP_HAS_NO_THREADS` we have a hard error). Removing this doesn't change anything but it avoids giving the impression that this is the definite way we want to handle those.

================
Comment at: libcxx/include/__thread/jthread.h:51
+    requires(!std::is_same_v<remove_cvref_t<_Fun>, jthread>)
+      : __stop_source_(), __thread_(__init_thread(std::forward<_Fun>(__fun), std::forward<_Args>(__args)...)) {
+    static_assert(is_constructible_v<decay_t<_Fun>, _Fun>);
----------------
I might suggest making `__init_thread` a static member function and passing `__stop_source_` to it explicitly. Otherwise we're calling a member function at a point where the object isn't fully initialized yet, which seems questionable. I don't think there's any real issue here, it's more a matter of style / clarity.

================
Comment at: libcxx/test/std/thread/thread.jthread/cons.func.token.pass.cpp:150
+
+  // TODO: test the following:
+  //
----------------
Let's add a link to https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-8  in this comment.

Also, if you don't have a concrete idea of how this might be done (portably etc..), let's not make this a TODO. If we don't realistically plan on fixing it, it shouldn't be a TODO.

We can say instead:

```
// Per https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-8:
// 
// Throws: system_error if unable to start the new thread.
// Error conditions:
// resource_unavailable_try_again - the system lacked the necessary resources to create another thread,
// or the system-imposed limit on the number of threads in a process would be exceeded.
//
// Unfortunately, this is extremely hard to test portably so we don't have a test for this error condition right now.
```

================
Comment at: libcxx/test/std/thread/thread.jthread/cons.move.pass.cpp:34
+    std::jthread j2(std::move(j1));
+    assert(j2.get_id() == id1);
+  }
----------------
https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-10 says that `j1.get_id()` should be `id()`, so we might as well assert that.

================
Comment at: libcxx/test/std/thread/thread.jthread/detach.pass.cpp:36
+      done = true;
+    }};
+
----------------
To troubleshoot whether the CI issue is that this curly brace is taking forever to complete and then the main thread finishes before the detached thread is done, you could insert a sleep at the end of the test. If that is indeed the issue, I can't think of another way to solve this problem since you don't have any handle to the thread once you've detached it. It's kind of lame but it might be acceptable to insert a short sleep with a comment at the end if it reliably solves this issue.

================
Comment at: libcxx/test/std/thread/thread.jthread/detach.pass.cpp:69-71
+  }
+
+#endif
----------------

================
Comment at: libcxx/test/std/thread/thread.jthread/dtor.pass.cpp:62-63
+    // be less than numberOfThreads.
+    // This is not going to catch issues 100%. Creating more threads to increase
+    // the probability of catching the issue
+    assert(calledTimes.load(std::memory_order_relaxed) == numberOfThreads);
----------------

================
Comment at: libcxx/test/std/thread/thread.jthread/get_id.pass.cpp:23-28
+template <class T>
+concept IsGetIdNoexcept = requires(const T& a) {
+  { a.get_id() } noexcept;
+};
+
+static_assert(IsGetIdNoexcept<std::jthread>);
----------------
Suggestion:

```
static_assert(noexcept(std::declval<std::jthread>().get_id()));
```

This seems a bit simpler. This applies in a few other tests as well.

================
Comment at: libcxx/test/std/thread/thread.jthread/get_stop_token.pass.cpp:37
+    auto ss                               = jt.get_stop_source();
+    std::same_as<std::stop_token> auto st = std::as_const(jt).get_stop_token();
+
----------------
That way if we returned a reference, the test would fail. You probably want to do this in the other tests too.

================
Comment at: libcxx/test/std/thread/thread.jthread/join.deadlock.pass.cpp:13
+
+// TSAN bug: TODO: fill the link
+// UNSUPPORTED: tsan
----------------
This should be a link to the github issue!

================
Comment at: libcxx/test/std/thread/thread.jthread/join.deadlock.pass.cpp:36
+int main(int, char**) {
+#if !defined(TEST_HAS_NO_EXCEPTIONS)
+  // resource_deadlock_would_occur - if deadlock is detected or get_id() == this_thread::get_id().
----------------
We could do `UNSUPPORTED: no-exceptions` instead.

================
Comment at: libcxx/test/std/thread/thread.jthread/joinable.pass.cpp:39-44
+  // Non-default constructed
+  {
+    const std::jthread jt{[]{}};
+    std::same_as<bool> auto result = jt.joinable();
+    assert(result);
+  }
----------------
I think we should check two cases here:

1. The thread of execution has finished running before we call `jt.joinable()`, and
2. the thread of execution has not finished running yet before we call `jt.joinable()`.

You can use an atomic variable to signal that.

================
Comment at: libcxx/test/std/thread/thread.jthread/typedef.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I would suggest renaming this to `types.compile.pass.cpp`, I think it is a bit more consistent with the rest of the test suite.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151559