[PATCH] D46140: [coroutines] Add std::experimental::task<T> type
Casey Carter via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 27 11:02:59 PDT 2019
CaseyCarter requested changes to this revision.
CaseyCarter added a subscriber: mclow.lists.
CaseyCarter added inline comments.
This revision now requires changes to proceed.
================
Comment at: include/experimental/__memory:80
+{
+ return (__s + __a - 1) & ~(__a - 1);
+}
----------------
This is missing preconditions that `__a` is a power of 2, and that `__s <= -__a`.
================
Comment at: include/experimental/task:141
+
+ void* __pointer = __charAllocator.allocate(
+ __get_padded_frame_size_with_allocator<_CharAlloc>(__size));
----------------
The return value of `allocate` isn't necessarily convertible to `void*`, it could be a fancy pointer. We should either `static_assert(is_same_v<typename allocator_traits<_CharAlloc>::void_pointer, void*>, "Piss off with your fancy pointers");` or use `pointer_traits` here and in `__deallocFunc` to unfancy and re-fancy the pointer.
================
Comment at: include/experimental/task:210
+private:
+ friend struct __task_promise_final_awaitable;
+
----------------
Pure style comment: I recommend using the non-elaborated `friend __task_promise_final_awaitable;` whenever possible. `friend struct foo` declares or redeclares `struct foo` in the enclosing namespace, whereas `friend foo` uses name lookup to find `foo` and makes it a friend. The latter form makes it far easier to analyze compiler errors when you screw something up in maintenance. (And on 480)
================
Comment at: include/experimental/task:220
+public:
+ __task_promise() _NOEXCEPT : __state_(_State::__no_value) {}
+
----------------
This mem-initializer for `__state_` is redundant with the default member initializer on 306.
================
Comment at: include/experimental/task:227
+ break;
+#ifndef _LIBCPP_NO_EXCEPTIONS
+ case _State::__exception:
----------------
I suggest moving the `case` label and `break` outside the `#ifndef` here so the compiler won't warn about this case being unhandled when `_LIBCPP_NO_EXCEPTIONS`.
================
Comment at: include/experimental/task:244
+ exception_ptr(current_exception());
+ __state_ = _State::__exception;
+#else
----------------
Are you certain that `unhandled_exception` can't possibly be called after storing a value? If so, this would leak the value.
================
Comment at: include/experimental/task:254
+ template <typename _Value,
+ enable_if_t<is_convertible<_Value, _Tp>::value, int> = 0>
+ void return_value(_Value&& __value)
----------------
Style: you've been using the `_v` variable templates for traits elsewhere.
================
Comment at: include/experimental/task:261
+ template <typename _Value>
+ auto return_value(std::initializer_list<_Value> __initializer) _NOEXCEPT_(
+ (is_nothrow_constructible_v<_Tp, std::initializer_list<_Value>>))
----------------
Unnecessary `std::` qualifier. (Occurs repeatedly.)
================
Comment at: include/experimental/task:263
+ (is_nothrow_constructible_v<_Tp, std::initializer_list<_Value>>))
+ -> std::enable_if_t<
+ std::is_constructible_v<_Tp, std::initializer_list<_Value>>> {
----------------
Style: the use of trailing-return-type SFINAE here is inconsistent with the use of template parameter SFINAE on 254.
================
Comment at: include/experimental/task:308
+ union {
+ char __empty_;
+ _Tp __value_;
----------------
These `__empty_` members seem extraneous.
================
Comment at: include/experimental/task:309
+ char __empty_;
+ _Tp __value_;
+ exception_ptr __exception_;
----------------
Should we `static_assert` that `_Tp` is a destructible object type?
================
Comment at: include/experimental/task:373
+template <>
+class __task_promise<void> final : public __task_promise_base {
+ using _Handle = coroutine_handle<__task_promise>;
----------------
Do we care about `task<cv-void>`?
================
Comment at: include/experimental/task:389
+
+ void __rvalue_result() { __throw_if_exception(); }
+
----------------
Should these `__foovalue_result` members be `foo`-qualified to make them harder to misuse?
================
Comment at: include/experimental/task:407
+ using promise_type = __task_promise<_Tp>;
+
+private:
----------------
Similar to above, should we `static_assert` that `_Tp` is `void`, an lvalue reference type, or a destructible non-array object type?
================
Comment at: include/experimental/task:453
+ decltype(auto) await_resume() {
+ return this->__coro_.promise().__lvalue_result();
+ }
----------------
`this->` is extraneous in these classes that derive from the concrete `_AwaiterBase`. (And on 470)
================
Comment at: include/experimental/task:458
+ _LIBCPP_ASSERT(__coro_,
+ "Undefined behaviour to co_await an invalid task<T>");
+ return _Awaiter{__coro_};
----------------
Is missing a subject. How about "co_await on an invalid task<T> has undefined behavior"? (And on 475)
================
Comment at: include/experimental/task:28
+#if defined(_LIBCPP_WARNING)
+_LIBCPP_WARNING("<experimental/task> cannot be used with this compiler")
+#else
----------------
"requires a compiler with support for coroutines" would be more informative.
================
Comment at: test/std/experimental/task/awaitable_traits.hpp:17
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_COROUTINES
+
----------------
It's super sketchy for test code to inject things into the production namespace and to use reserved names. It's also inappropriate to use libc++ internals in a test in the `std` tree. (Occurs repeatedly.)
================
Comment at: test/std/experimental/task/counted.hpp:81
+
+ static std::size_t nextId_;
+ static std::size_t defaultConstructedCount_;
----------------
IIUC that the tests all require C++17, you could declare these `inline` and avoid the external definitions / `DEFINE_COUNTED_VARIABLES` macro.
================
Comment at: test/std/experimental/task/counted.hpp:90
+#define DEFINE_COUNTED_VARIABLES() \
+ std::size_t counted::nextId_; \
+ std::size_t counted::defaultConstructedCount_; \
----------------
Should `nextId_` be initialized to `1`, as it is in `reset` on 40?
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:18
+// manual_reset_event is a coroutine synchronisation tool that allows one
+// coroutine to await the event object and if the event was not crrently
+// in the 'set' state then will suspend the awaiting coroutine until some
----------------
"currently". Also, "was not currently" is weird.
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:23
+{
+ friend class _Awaiter;
+
----------------
Here's a great example of why elaborated `friend` can be weird. This doesn't make the `_Awaiter` class defined on 25 a friend - which needn't be a friend, it's nested so it has full access to `manual_reset_event` - it declares a class named `_Awaiter` in the enclosing namespace that is a friend.
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:25
+
+ class _Awaiter
+ {
----------------
Ditto "using reserved names in test code". (Occurs repeatedly.)
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:40
+ {
+ _LIBCPP_ASSERT(
+ __event_->__state_.load(std::memory_order_relaxed) !=
----------------
`_LIBCPP_ASSERT` is inappropriate in `std` test code; it's undefined when the library under test is not libc++. Use `LIBCPP_ASSERT` (with no leading underscore) from `test/support/test_macros.h` to mean "this assertion must hold only if the library under test is libc++" and `assert` if the assertion must hold regardless of the tested library. (Occurs repeatedly.)
(Ignore the misuses of `_LIBCPP_ASSERT` that @mclow.lists put in the `span` tests; I'll fix them when we implement `span` and enable the tests.)
EDIT: Actually, I'll fix them now to save myself the trouble of figuring out why the tests are broken later.
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:47
+
+ __event_->__awaitingCoroutine_ = __coro;
+
----------------
Why are we concerned about data races on `__event_->__state_`, but not `__event_->__awaitingCoroutine_`?
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:49
+
+ // If the compare-exchange fails then this means that the event was
+ // already 'set' and so we should not suspend - this code path requires
----------------
s/this means that//; s/and so we/so we/; s/suspend - this/suspend. This/ (Ok, I'm nitpicking, but I actually did have issues parsing this comment.)
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:86
+ // so that we have visibility of the writes to the coroutine frame in
+ // the current thrad before we resume it.
+ // Also needs to be 'release' in case the old value was 'not-set' so that
----------------
thread
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:88
+ // Also needs to be 'release' in case the old value was 'not-set' so that
+ // another thread that subsequently awaits the
+ _State oldState = __state_.exchange(_State::__set, std::memory_order_acq_rel);
----------------
"awaits the" what? The suspense is killing me!
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:92
+ {
+ _VSTD::exchange(__awaitingCoroutine_, {}).resume();
+ }
----------------
`std::exchange`
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:102
+
+ // Note, we use 'relaxed' memory order here since it considered a
+ // data-race to call reset() concurrently either with operator co_await()
----------------
What's the antecedent of "it"?
================
Comment at: test/std/experimental/task/sync_wait.hpp:36-37
+ __isSet_ = true;
+ }
+ __cv_.notify_all();
+ }
----------------
lewissbaker wrote:
> The call to `notify_all()` needs to be inside the lock.
> Otherwise, it's possible the waiting thread may see the write to `__isSet_` inside `wait()` below and return, destroying the `condition_variable` before `__cv_.notify_all()` call completes.
The write to `__isSet_` must be under the lock, the call to `notify_all` need not be. (Not that I care either way, just clarifying.)
================
Comment at: test/std/experimental/task/sync_wait.hpp:14
+
+#include <experimental/__config>
+#include <experimental/coroutine>
----------------
There's no such header in the working draft or the Coroutines TS.
================
Comment at: test/std/experimental/task/sync_wait.hpp:70
+
+ friend struct _FinalAwaiter;
+
----------------
Ditto "doesn't do what you think it does and is unnecessary."
================
Comment at: test/std/experimental/task/sync_wait.hpp:115
+ break;
+#ifndef _LIBCPP_NO_EXCEPTIONS
+ case _State::__exception:
----------------
Ditto "not in test code". You should include `"test_macros.h"` and use `TEST_NO_EXCEPTIONS`. (Occurs repeatedly.)
================
Comment at: test/std/experimental/task/sync_wait.hpp:135
+#ifndef _LIBCPP_NO_EXCEPTIONS
+ ::new (static_cast<void*>(&__exception_)) exception_ptr(
+ std::current_exception());
----------------
`#include<new>` for the definition of True Placement New. (This may be missing above as well, I don't recall or have time to scroll back.)
================
Comment at: test/std/experimental/task/sync_wait.hpp:139
+#else
+ _VSTD::abort();
+#endif
----------------
Here's another `_VSTD` that should be `std`. Please audit all test code.
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:18
+#include <memory>
+#include <experimental/memory_resource>
+
----------------
This is not a Standard or Coroutines TS header.
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:26
+{
+ static size_t allocator_instance_count = 0;
+
----------------
`static` is extraneous in an anonymous namespace.
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:37
+ using difference_type = std::ptrdiff_t;
+ using is_always_equal = std::false_type;
+
----------------
These are the defaults for `size_type`, `difference_type`, and `is_always_equal`; you should omit them to verify that the tested code properly uses `allocator_traits`.
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:54
+ my_allocator(my_allocator&& other)
+ : totalAllocated_(std::move(other.totalAllocated_))
+ {
----------------
This is almost certainly going to cause problems, since it leaves the moved-from allocator invalid. (And on 68.)
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:75
+ {
+ --allocator_instance_count;
+ }
----------------
I suggest `assert(allocator_instance_count > 0)` before decrementing. We're testing code that explicitly destroys things, we should make sure it doesn't destroy more things than it constructs.
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:78
+
+ char* allocate(size_t n) {
+ const auto byteCount = n * sizeof(T);
----------------
`allocate` needs to return `T*`. (Also, `std::size_t`.) (And on 88)
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:90
+ const auto byteCount = n * sizeof(T);
+ *totalAllocated_ -= byteCount;
+ std::free(p);
----------------
Here again, I suggest an assertion that we're not freeing more bytes than have been allocated.
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:221
+
+int main()
+{
----------------
In libc++ tests `main` is always declared `int main(int, char**)` (and explicitly returns `0`) to support the freestanding work. (Occurs repeatedly.)
================
Comment at: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp:46
+ // calling the move-constructor. This move could also potentially be
+ // elided by the
+ assert(counted::move_constructor_count() <= 1);
----------------
Again, leaving me in suspense.
Repository:
rCXX libc++
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D46140/new/
https://reviews.llvm.org/D46140
More information about the cfe-commits
mailing list