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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/Cxx20.rst:52
    .. [#note-P2231] P2231: Optional is complete. The changes to variant haven't been implemented yet.
+   .. [#note-P0660] P0660: Section 32.3 Stop Tokens is complete. ``jthread`` haven't been implemented yet.
 
----------------



================
Comment at: libcxx/include/__stop_token/stop_callback.h:32-37
+  static_assert(invocable<_Callback>,
+                "Mandates: stop_callback is instantiated with an argument for the template parameter Callback that "
+                "satisfies both invocable and destructible.");
+  static_assert(destructible<_Callback>,
+                "Mandates: stop_callback is instantiated with an argument for the template parameter Callback that "
+                "satisfies both invocable and destructible.");
----------------
That will make it easier for a user to find out what the problem is.


================
Comment at: libcxx/include/__stop_token/stop_callback.h:67
+        // st.stop_requested() was false and this is successfully added to the linked list
+        __st.__ref_counted_state_.__swap(__ref_counted_state_);
+      }
----------------
I think this is equivalent but clearer:

```
__ref_counted_state_ = std::move(__st.__ref_counted_state_);
```

So then I think we can probably add the following private constructor and then delegate to it from both these public constructors:

```
private:
struct __private_tag { };
template <class _RefCountedState, class _Cb>
_LIBCPP_HIDE_FROM_ABI explicit stop_callback(__private_tag, _RefCountedState&& __state, _Cb&& __cb) noexcept(is_nothrow_constructible_v<_Callback, _Cb>)
      : __stop_callback_base(&stop_callback::__callback_fn_impl),
        __callback_(std::forward<_Cb>(__cb)),
        __ref_counted_state_() {
  if (...) if (...) __ref_counted_state_ = std::forward<...>(...);
}

public:
template <class _Cb>
    requires constructible_from<_Callback, _Cb>
_LIBCPP_HIDE_FROM_ABI explicit stop_callback(stop_token&& __st,
                                               _Cb&& __cb) noexcept(is_nothrow_constructible_v<_Callback, _Cb>)
      : stop_callback(__private_tag{}, std::move(__st.__state_), std::forward(__cb)) { }

// same for const ref ctor
```

And at that point, you have only one reference to `__callback_fn_impl`, so you could also just initialize `__stop_callback_base` inline with a stateless lambda if that makes the code clearer because more localized.


================
Comment at: libcxx/include/__stop_token/stop_callback.h:90
+  _LIBCPP_HIDE_FROM_ABI static void __callback_fn_impl(__stop_callback_base* __cb_base) noexcept {
+    std::forward<_Callback>(static_cast<stop_callback*>(__cb_base)->__callback_)();
+  }
----------------
I would add a comment here explaining that the stop callback is supposed to only be called once because we can't *guarantee* that this isn't a rvalue ref.


================
Comment at: libcxx/include/__stop_token/stop_source.h:37
+
+  _LIBCPP_HIDE_FROM_ABI explicit stop_source(nostopstate_t) noexcept {}
+
----------------



================
Comment at: libcxx/include/__stop_token/stop_source.h:49-59
+    if (__ref_counted_state_ != __other.__ref_counted_state_) {
+      auto __tmp           = std::move(__ref_counted_state_);
+      __ref_counted_state_ = __other.__ref_counted_state_;
+      if (__ref_counted_state_.__has_state()) {
+        __ref_counted_state_.__get()->__increment_stop_source_counter();
+      }
+      if (__tmp.__has_state()) {
----------------
Is this equivalent?

```
if (__ref_counted_state_ != __other.__ref_counted_state_) {
  if (__ref_counted_state_)
    __ref_counted_state_->__decrement_stop_source_counter();
  __ref_counted_state_ = __other.__ref_counted_state_;
  if (__ref_counted_state_)
    __ref_counted_state_->__increment_stop_source_counter();
}
return *this;
```

Or even this?

```
// NOTE: We increment before we decrement to make sure that we don't hit 0 by mistake in case of self-assignment.
if (__other.__ref_counted_state_)
  __other.__ref_counted_state_->__increment_stop_source_counter();
if (__ref_counted_state_)
    __ref_counted_state_->__decrement_stop_source_counter();
__ref_counted_state_ = __other.__ref_counted_state_;
return *this;
```



================
Comment at: libcxx/include/__stop_token/stop_source.h:71
+  _LIBCPP_HIDE_FROM_ABI void swap(stop_source& __other) noexcept {
+    __ref_counted_state_.__swap(__other.__ref_counted_state_);
+  }
----------------
Should the `__intrusive_shared_ptr` be `std::` swappable? Probably?


================
Comment at: libcxx/include/__stop_token/stop_source.h:83-86
+    if (!__ref_counted_state_.__has_state()) {
+      return false;
+    }
+    return __ref_counted_state_.__get()->__request_stop();
----------------



================
Comment at: libcxx/include/__stop_token/stop_source.h:94
+private:
+  __ref_counted_stop_state __ref_counted_state_;
+};
----------------
With the shared-ptr proposed change, this would become something like `__intrusive_shared_ptr<__stop_state> __state_`, which IMO is easier to understand since we immediately understand "okay this thing is basically a shared ptr".


================
Comment at: libcxx/include/__stop_token/stop_state.h:42
+
+  // "stop_source counter" is not used for lifetime reference counter.
+  // When number of stop_source reaches 0, the remaining stop_tokens's
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:46
+  //
+  // "callback list locked" bit is for implementing the pesudo-mutex to guard the operations
+  // on the callback list
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:115
+
+  _LIBCPP_HIDE_FROM_ABI __callback_list_lock __try_lock_for_request_stop() noexcept {
+    // If it is already stop_requested, do not try to request stop or lock the list again.
----------------
Can this be a `private` member?


================
Comment at: libcxx/include/__stop_token/stop_state.h:226-228
+  _LIBCPP_HIDE_FROM_ABI __stop_state* __get() const noexcept { return __stop_state_; }
+
+  _LIBCPP_HIDE_FROM_ABI bool __has_state() const noexcept { return __stop_state_ != nullptr; }
----------------
Since this is really close to a shared pointer, I think it would make sense to define `operator*`, `operator->` and `explicit operator bool` instead of these two. And also probably `operator<=>(nullptr_t)` so you can compare explicitly against `nullptr` and have that be equivalent to `operator bool`.


================
Comment at: libcxx/include/__stop_token/stop_state.h:240-250
+  // the memory order for increment/decrement the counter is the same for shared_ptr
+  // increment is relaxed and decrement is acq_rel
+  _LIBCPP_HIDE_FROM_ABI void __increment_ref_count(__stop_state* __state) {
+    __state->__ref_count_.fetch_add(1, std::memory_order_relaxed);
+  }
+
+  _LIBCPP_HIDE_FROM_ABI void __decrement_ref_count(__stop_state* __state) {
----------------
I think it would be possible to extract that logic out. Then, `__ref_counted_stop_state` would become something like `__intrusive_shared_ptr` or `__basic_shared_ptr`. The idea would be to assume that the refcount is in the value you point to and to parameterize on the operation used to manipulate (increment/decrement) it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145183