[libcxx-commits] [PATCH] D150205: [libc++] Utilities for implementing stop_token

[Louis Dionne via Phabricator via libcxx-commits]

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

I really like this! Thanks a lot for splitting this into another review, I think this makes the `stop_token` code much easier to understand. LGTM with some comments.



================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:23-27
+/**
+ * This class implements an RAII unique_lock without a mutex.
+ * It uses std::atomic<State> where State contains a lock bit
+ * and might contain other data.
+ */
----------------
Nit: let's use a c++ style comment


================
Comment at: libcxx/include/__stop_token/intrusive_list.h:23
+#if _LIBCPP_STD_VER >= 20
+
+template <class _Derived>
----------------
Note for yourself: you think it might be possible to instead own the elements of the list and simplify some stuff in stop_callback?


================
Comment at: libcxx/include/__stop_token/intrusive_list.h:29
+};
+
+template <class _Node>
----------------
Maybe a short comment explaining what the class does? Mention that it doesn't own the nodes.


================
Comment at: libcxx/include/__stop_token/intrusive_list.h:31
+template <class _Node>
+struct __intrusive_list {
+  _LIBCPP_HIDE_FROM_ABI bool __empty() const noexcept { return __head_ == nullptr; }
----------------
Currently, the list can be copied or copy-assigned. I think this is a bit misleading, since the list doesn't really own the elements. Normally, we expect that two copies of an object are independent, which is not the case here since it's not a value type. In a way, this is some kind of "view" like `string_view`. The "structure" created by the list elements exists regardless of whether we have an `__intrusive_list` object alive somewhere, and the `__intrusive_list` only gives us an API to manipulate it along with a handle to the first element in that "structure".

So I would suggest one of two things:
1. Make it non-copyable, and use
```
__intrusive_list() = default;
__intrusive_list(__intrusive_list const&) = delete;
__intrusive_list(__intrusive_list&&) = default;
__intrusive_list& operator=(__intrusive_list const&) = delete;
__intrusive_list& operator=(__intrusive_list&&) = default;
```
2. Change the name slightly to something like `__intrusive_list_view` (??) to make it clearer that it's a view. Along with a comment explaining the design of the class, I think this can go a long way. Then you probably also want to make the special members explicit like above with `= default` for everything, just to call it out. And finally, if you go for that approach, don't forget to update file names, header guards and the test!

Edit: Since the destructor doesn't destroy the elements in the list, I think we now agree that (2) is probably the way to go.


================
Comment at: libcxx/include/__stop_token/intrusive_shared_ptr.h:28-32
+// Specialize __intrusive_shared_ptr_traits<_Tp> and implements
+//
+// static std::atomic<U>& __get_atomic_ref_count(_Tp&);
+//
+// where U must be an integral type
----------------



================
Comment at: libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp:25
+
+using Lock = std::__atomic_unique_lock<uint8_t, uint8_t(1)>;
+
----------------
Can you also test when the lock bit is not the first one? It should be easy to do something like:

```
template <uint8_t LockBit>
void test() {
  // all the stuff
}

int main(...) {
  test<1>();
  test<2>();
  test<3>();
  // etc..
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150205