[libcxx-commits] [PATCH] D145183: [libc++] Implement `stop_token`

Fri Apr 28 09:32:41 PDT 2023

ldionne added a comment.

Could you extract `atomic_unique_lock.h`, `intrusive_list.h` and `intrusive_shared_ptr.h` into a separate review? I think we can check those in soon and that will greatly simplify this review. I don't know where to put them. It's not great, but I'd suggest leaving them in `__stop_token` and we can think about reorganization later.

================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Since this is general purpose, it makes little sense to keep this under `__stop_token/`, but we also don't have any good place for these general-purpose utilities. No action item, but let's keep this at the back of our mind that we might need to reorganize things a bit in the future.

================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:4
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Can you add just a few simple tests for this class under `libcxx/test/libcxx`?

================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:40
+  template <class _Pred>
+  _LIBCPP_HIDE_FROM_ABI __atomic_unique_lock(std::atomic<_State>& __state, _Pred&& __fail_to_lock) noexcept
+      : __state_(__state), __is_locked_(false) {
----------------
Would it make sense to use `std::try_to_lock_t` here to convey that we're taking the lock? Otherwise, this can be confusing with `std::unique_lock`.

================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:66
+
+  _LIBCPP_HIDE_FROM_ABI bool __is_successfully_locked() const noexcept { return __is_locked_; }
+
----------------
Maybe we could call this `__owns_lock()` to mirror `unique_lock`?

================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:87
+  _LIBCPP_HIDE_FROM_ABI bool __lock_impl(
+      _Pred&& __fail_to_lock, _UnaryFunction&& __state_after_lock, std::memory_order __locked_ordering) noexcept {
+    // At this stage, until we exit the inner while loop, other than the atomic state, we are not reading any order
----------------
I think a short comment explaining what `__fail_to_lock` and `__state_after_lock` are used for, it's not obvious from the names alone.

Actually, maybe `__fail_to_lock` should be called `__give_up_locking`? `__fail_to_lock` can sound like something that is called in case you've failed to take the lock, which is not quite correct.

================
Comment at: libcxx/include/__stop_token/atomic_unique_lock.h:117
+                                             // Some use cases need to set other bits at the same time as an atomic
+                                             // operation therefore we accepts a function
+        __locked_ordering,        // sucessful exchange order. Usually it should be std::memory_order_acquire.
----------------

================
Comment at: libcxx/include/__stop_token/intrusive_list.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Can you add just a few simple tests for this class under `libcxx/test/libcxx`?

================
Comment at: libcxx/include/__stop_token/intrusive_list.h:31
+struct __intrusive_list {
+  _LIBCPP_HIDE_FROM_ABI bool __has_node() const noexcept { return __head_ != nullptr; }
+
----------------
`__empty()`? Or `__is_empty()`?

================
Comment at: libcxx/include/__stop_token/intrusive_list.h:42-47
+    _Node* __front = __head_;
+    __head_        = __head_->__next_;
+    if (__head_) {
+      __head_->__prev_ = nullptr;
+    }
+    return __front;
----------------
I think this is fine, but you should add a comment explaining that it's fine not to set `__front->__next_ = nullptr`, since it's not part of the linked list anymore.

================
Comment at: libcxx/include/__stop_token/intrusive_list.h:57-59
+    } else if (__node == __head_) {
+      __pop_front();
+    }
----------------
If we don't have a prev, then we are necessarily the `head`. I would instead change this to:

```
if (...) { ... } else {
  _LIBCPP_ASSERT(__node == __head_, "message");
  __pop_front();
}
```

IOW, I think this function needs the precondition that `__node` is in the list, similarly to how `std::list` requires the iterator to be from this list when you call `erase()`. I think that's a really useful and important model to have.

================
Comment at: libcxx/include/__stop_token/intrusive_shared_ptr.h:28-29
+
+template <class _Tp>
+struct __intrusive_shared_ptr_traits;
+
----------------
Can you please add a comment documenting what this type is and what its contents should be for anyone specializing it?

================
Comment at: libcxx/include/__stop_token/intrusive_shared_ptr.h:31-32
+
+template <class _Tp>
+struct __intrusive_shared_ptr {
+  _LIBCPP_HIDE_FROM_ABI __intrusive_shared_ptr() = default;
----------------
Let's also add a short comment documenting this class, since it's general purpose in nature and we might reuse it in the future.

================
Comment at: libcxx/include/__stop_token/intrusive_shared_ptr.h:35
+
+  _LIBCPP_HIDE_FROM_ABI __intrusive_shared_ptr(_Tp* __raw_ptr) : __raw_ptr_(__raw_ptr) {
+    if (__raw_ptr_)
----------------
I'd make this `explicit`, like the one for `std::shared_ptr`.

================
Comment at: libcxx/include/__stop_token/stop_callback.h:84
+        __state_() {
+    if (__state) {
+      if (__state->__add_callback(this)) {
----------------
Isn't this always `nullptr` since you default initialize it above?

================
Comment at: libcxx/include/__stop_token/stop_state.h:150
+
+    // under below condition, the request_stop call just poped __cb from the list and could execute it now
+    bool __potentially_executing_now = __cb->__prev_ == nullptr && !__callback_list_.__is_head(__cb);
----------------

================
Comment at: libcxx/include/__stop_token/stop_state.h:153
+
+    __callback_list_.__remove(__cb);
+
----------------
I think we shouldn't call this `__remove` if we know that `__cb` is not in the list anymore, which can be checked via `__cb->__prev_ == nullptr && !__callback_list_.__is_head(__cb)`. Oh and that's exactly the check you make above with `__potentially_executing_now`!

And now I think we have the precondition we want for `__remove`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145183