[libcxx-commits] [PATCH] D153441: [libc++] Implement `std::condition_variable_any::wait[_for/until]` overloads that take `stop_token`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good. I have some minor comments. I think it would be good to keep track how often predicates are called and whether that matches the (minimal) expectation. For example when a `stop_requested() == true` the predicate is called exactly one time.



================
Comment at: libcxx/include/condition_variable:106
+    template <class Lock, class Predicate>
+      bool wait(Lock& lock, stop_token stoken, Predicate pred);         // since C++20
+
----------------
Can you align the `// since C++20` ?


================
Comment at: libcxx/include/condition_variable:318
+            return true;
+        if (wait_until(__lock, __abs_time) == cv_status::timeout)
+            return __pred();
----------------



================
Comment at: libcxx/include/condition_variable:327
+    _Lock& __lock, stop_token __stoken, const chrono::duration<_Rep, _Period>& __rel_time, _Predicate __pred) {
+    return wait_until(__lock, std::move(__stoken), chrono::steady_clock::now() + __rel_time, std::move(__pred));
+}
----------------



================
Comment at: libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp:95
+
+    auto oldTime = std::chrono::steady_clock::now();
+
----------------
This is our typical code style.


================
Comment at: libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp:134
+    auto thread           = support::make_test_thread([&]() {
+      std::this_thread::sleep_for(2ms);
+      ss.request_stop();
----------------
I wonder here and at other places whether 2 ms is long enough.
Are we sure this is changed after the `wait_for` is really in it's loop? Or can the flag be set before entering the loop.

Would it make sense to validate how often the predicate is executed? When it's less than 2 the loop was executed 0 times.


================
Comment at: libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_token_pred.pass.cpp:42
+    // [Note 1: The returned value indicates whether the predicate evaluated to true regardless of whether there was a stop request.]
+    std::same_as<bool> auto r1 = cv.wait(lock, ss.get_token(), []() { return false; });
+    assert(!r1);
----------------
Please include `<concepts>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153441