[PATCH] D46140: [coroutines] Add std::experimental::task<T> type
Lewis Baker via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 29 16:56:31 PDT 2019
lewissbaker marked 40 inline comments as done.
lewissbaker added a comment.
Thanks very much for the detailed review @CaseyCarter! Very much appreciated :)
================
Comment at: include/experimental/__memory:80
+{
+ return (__s + __a - 1) & ~(__a - 1);
+}
----------------
CaseyCarter wrote:
> This is missing preconditions that `__a` is a power of 2, and that `__s <= -__a`.
Is there a recommended way of documenting/implementing these preconditions on a constexpr function in libc++?
The previous version that lived in `<experimental/memory_resource>` was not marked constexpr and so was able to use `_LIBCPP_ASSERT`.
================
Comment at: include/experimental/task:28
+#if defined(_LIBCPP_WARNING)
+_LIBCPP_WARNING("<experimental/task> cannot be used with this compiler")
+#else
----------------
CaseyCarter wrote:
> "requires a compiler with support for coroutines" would be more informative.
This messaging was copied from <experimental/coroutine>.
I'll update the message there as well.
================
Comment at: include/experimental/task:141
+
+ void* __pointer = __charAllocator.allocate(
+ __get_padded_frame_size_with_allocator<_CharAlloc>(__size));
----------------
CaseyCarter wrote:
> 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.
I'm not quite sure how to go about using `pointer_traits` here when interacting with coroutines and allocators.
Under C++20 I guess I would do something like:
```
typename allocator_traits<_CharAlloc>::void_pointer __pointer = __charAllocator.allocate(...);
...
return _VSTD::to_address(__pointer);
```
I assume that `operator new()` still needs to return `void*` rather than the fancy pointer so we need the `to_address` call in there. Is there some fallback for `to_address()` for unfancying the pointer in earlier standard versions?
Maybe we should just go with the `static_assert()` for now?
================
Comment at: include/experimental/task:220
+public:
+ __task_promise() _NOEXCEPT : __state_(_State::__no_value) {}
+
----------------
CaseyCarter wrote:
> This mem-initializer for `__state_` is redundant with the default member initializer on 306.
I'll remove the default member initializer on 306 then.
We can't `= default` the constructor here due to the union member containing types with non-trivial constructors/destructors.
================
Comment at: include/experimental/task:244
+ exception_ptr(current_exception());
+ __state_ = _State::__exception;
+#else
----------------
CaseyCarter wrote:
> Are you certain that `unhandled_exception` can't possibly be called after storing a value? If so, this would leak the value.
Yes, I think there is a case where this could happen.
If we execute a `co_return someValue` which calls `promise.return_value()` and constructs `__value_`.
Then while executing the `goto final_suspend;` if any of the destructors of in-scope variables throw an exception which then escapes the coroutine body we could end up calling `unhandled_exception()` with `__value_` already having been constructed.
Similarly, the exception thrown from a destructor could be caught and then a new `co_return someOtherValue;` executed.
So we should probably be guarding against `__value_` containing a value inside `return_value()` as well.
================
Comment at: include/experimental/task:308
+ union {
+ char __empty_;
+ _Tp __value_;
----------------
CaseyCarter wrote:
> These `__empty_` members seem extraneous.
The `__empty_` members were added at your request ;)
See https://reviews.llvm.org/D46140?id=144168#inline-403822
I can remove them if you think they're not necessary.
================
Comment at: include/experimental/task:309
+ char __empty_;
+ _Tp __value_;
+ exception_ptr __exception_;
----------------
CaseyCarter wrote:
> Should we `static_assert` that `_Tp` is a destructible object type?
Sure, will do.
I wonder if we should make this a requirement in the wording of [P1056](https://wg21.link/P1056)?
What do you think @GorNishanov?
================
Comment at: include/experimental/task:373
+template <>
+class __task_promise<void> final : public __task_promise_base {
+ using _Handle = coroutine_handle<__task_promise>;
----------------
CaseyCarter wrote:
> Do we care about `task<cv-void>`?
I don't feel strongly either way.
It's not something I've ever had a use for, but maybe it should be done for completeness?
================
Comment at: include/experimental/task:389
+
+ void __rvalue_result() { __throw_if_exception(); }
+
----------------
CaseyCarter wrote:
> Should these `__foovalue_result` members be `foo`-qualified to make them harder to misuse?
I'm not sure if it makes it much safer.
We already have 'rvalue' in the method name which gives a hint to its move-ness.
It's currently only called in one place:
```
return this->__coro_.promise().__rvalue_result();
```
would become:
```
return std::move(this->__coro_.promise()).__rvalue_result();
```
================
Comment at: include/experimental/task:407
+ using promise_type = __task_promise<_Tp>;
+
+private:
----------------
CaseyCarter wrote:
> Similar to above, should we `static_assert` that `_Tp` is `void`, an lvalue reference type, or a destructible non-array object type?
Sure, can do.
Further, to support `task<T>::operator co_await() &&` T needs to both be destructible and move-constructible.
For `task<T>::operator co_await() &` T only needs to be destructible.
================
Comment at: include/experimental/task:453
+ decltype(auto) await_resume() {
+ return this->__coro_.promise().__lvalue_result();
+ }
----------------
CaseyCarter wrote:
> `this->` is extraneous in these classes that derive from the concrete `_AwaiterBase`. (And on 470)
I get compile-errors if I remove the `this->`.
```
libcxx/include/experimental/task:502:16: error: 'std::experimental::coroutines_v1::task<counted>::__coro_' is not a member of class '_Awaiter'
return __coro_.promise().__rvalue_result();
```
Compiler bug?
================
Comment at: test/std/experimental/task/awaitable_traits.hpp:17
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_COROUTINES
+
----------------
CaseyCarter wrote:
> 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.)
These were intended to be eventually added to the <experimental/coroutine> header and under the std::experimental namespace which is why they were using these macros. See [P1288R0](https://wg21.link/P1288R0).
Similarly for `sync_wait()` which is proposed in [P1171R0](https://wg21.link/P1171R0).
Is there some common namespace that test utilities should be declared within?
If not I'll just move them to global scope and de-uglify them for now.
================
Comment at: test/std/experimental/task/counted.hpp:90
+#define DEFINE_COUNTED_VARIABLES() \
+ std::size_t counted::nextId_; \
+ std::size_t counted::defaultConstructedCount_; \
----------------
CaseyCarter wrote:
> Should `nextId_` be initialized to `1`, as it is in `reset` on 40?
The intent was that any test would always initialise these values by calling reset() before running.
I hadn't intended the static-initialization to "do the right thing" here but I can if you think that's preferable.
================
Comment at: test/std/experimental/task/manual_reset_event.hpp:47
+
+ __event_->__awaitingCoroutine_ = __coro;
+
----------------
CaseyCarter wrote:
> Why are we concerned about data races on `__event_->__state_`, but not `__event_->__awaitingCoroutine_`?
Only the awaiting thread writes to `__awaitingCoroutine_` and publishes this value when it writes to `__state_` by setting the state to `__not_set_waiting_coroutine`.
A thread that calls `.set()` will only read from `__awaitingCoroutine_` if, when it updates the `__state_` variable, sees that the previous state was `__not_set_waiting_coroutine`.
So the write the `__awaitingCoroutine_` should strictly happens-before any read from `__awaitingCoroutine_`.
================
Comment at: test/std/experimental/task/sync_wait.hpp:36-37
+ __isSet_ = true;
+ }
+ __cv_.notify_all();
+ }
----------------
CaseyCarter wrote:
> 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.)
The problem here is that once we write `true` to `__isSet_` and release the lock that the thread calling `wait()` can potentially return and the on to destroy the `__oneshot_event` object, leaving the current thread with a dangling reference to a destroyed `std::condition_variable`.
I ran into a similar problem in cppcoro.
See https://github.com/lewissbaker/cppcoro/issues/98#issuecomment-450695759
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:18
+#include <memory>
+#include <experimental/memory_resource>
+
----------------
CaseyCarter wrote:
> This is not a Standard or Coroutines TS header.
I can't `#include <memory_resource>` as libc++ doesn't currently provide that header.
How should I go about writing a test that makes sure that std::experimental::task works with `std::experimental::pmr::polymorphic_allocator` (or `std::pmr::polymorphic_allocator` if available)?
================
Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:54
+ my_allocator(my_allocator&& other)
+ : totalAllocated_(std::move(other.totalAllocated_))
+ {
----------------
CaseyCarter wrote:
> This is almost certainly going to cause problems, since it leaves the moved-from allocator invalid. (And on 68.)
Isn't a moved-from allocator left in a valid but unspecified state?
ie. you can't assume it's ok to use. but should be ok to destruct or assign to.
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