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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__stop_token/stop_source.h:48
+  _LIBCPP_HIDE_FROM_ABI stop_source& operator=(const stop_source& __other) noexcept {
+    if (__other.__state_) {
+      __other.__state_->__increment_stop_source_counter();
----------------
I would add a comment like:

```
// increment `other` first so that we don't hit 0 in case of self-assignment
```


================
Comment at: libcxx/include/__stop_token/stop_source.h:72-74
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI bool stop_requested() const noexcept {
+    return __state_ != nullptr && __state_->__stop_requested();
+  }
----------------
> A default constructed stop_token has no associated stop-state, and thus has not had stop requested.

I assume this is tested in `stop_requested.pass.cpp`?


================
Comment at: libcxx/include/__stop_token/stop_state.h:42-50
+  // "stop_source counter" is not used for lifetime reference counting.
+  // When number of stop_source reaches 0, the remaining stop_tokens's
+  // stop_possible will return false. We need this counter to track this
+  //
+  // "callback list locked" bit is for implementing the atomic_unique_lock to guard the operations
+  // on the callback list
+  //
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:53-55
+  // reference count for stop_token + stop_callback + stop_source
+  // When counter reaches zero, the state is destroyed
+  // It is used by __ref_counted_stop_state, put it here for better layout
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:82
+    // object that returns true.
+    // request_stop's compare_exchange_week has release which syncs with this acquire
+    return (__state_.load(std::memory_order_acquire) & __stop_requested_bit) != 0;
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:88-89
+    // [stoptoken.mem] false if "a stop request was not made and there are no associated stop_source objects"
+    // Todo: Can this be std::memory_order_relaxed as the standard does not say anything except not to introduce data
+    // race
+    __state_t __curent_state = __state_.load(std::memory_order_acquire);
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:123
+    // If it is already stop_requested. Do not try to request it again.
+    const auto __lock_fail_condition = [__cb](__state_t __state) {
+      if ((__state & __stop_requested_bit) != 0) {
----------------
Or something!


================
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);
----------------
ldionne wrote:
> 



================
Comment at: libcxx/test/std/thread/thread.stoptoken/stopcallback/cons.const.token.pass.cpp:11-12
+
+// This test requires the dylib support introduced in D68480, which shipped in macOS 11.0.
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
+
----------------
You can replace this by `// XFAIL: availability-synchronization_library-missing` now. Here and everywhere else.


================
Comment at: libcxx/test/std/thread/thread.stoptoken/stopcallback/dtor.pass.cpp:155
+    auto ptr =
+        std::make_unique<std::stop_callback<std::function<void()>>>(ss.get_token(), [&] { wrapper.sc_ = nullptr; });
+
----------------
Maybe a bit more explicit?


================
Comment at: libcxx/test/std/thread/thread.stoptoken/stopcallback/dtor.pass.cpp:157
+
+    wrapper.sc_ = std::move(ptr);
+    assert(wrapper.sc_ != nullptr);
----------------
Is there any reason why you don't initialize `sc_` directly with `std::make_unique<>`? That would make it clear that `ptr` is not actually meaningful other than a temporary variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145183