[coroutines] Add std::experimental::task<T> type

[lewissbaker]

Summary

Adds the coroutine std::experimental::task<T> type described in proposal P1056R0.
See https://wg21.link/P1056R0.

This implementation allows customization of the allocator used to allocate the
coroutine frame by passing std::allocator_arg as the first argument, followed by
the allocator to use.

This supports co_awaiting the same task multiple times. The second and
subsequent times it returns a reference to the already-computed value.

This diff also adds some implementations of other utilities that have potential for
standardization as helpers within the test/... area:

• sync_wait(awaitable) - See P1171R0
• manual_reset_event

Move the definition of the aligned_allocation_size helper function
from <experimental/memory_resource> to <experimental/memory>
so it can be more widely used without pulling in memory_resource.

Outstanding work:

• Use C++14 keywords directly rather than macro versions eg. use noexcept instead of _NOEXCEPT).
• Add support for overaligned coroutine frames. This may need wording in the Coroutines TS to support passing the extra std::align_val_t.
• Eliminate use of if constexpr if we want it to compile under C++14.

[CaseyCarter]

include/experimental/task

57

Why qualify size_t here when you're in a subnamespace of std? (It's only necessary to qualify *functions* to avoid unintended ADL, not *types*.)

Why not _NOEXCEPT?

69

Ditto extraneous _VSTD. (I'll stop commenting these now.)

83

Is it an option to store struct foo : _Alloc { _DeallocFunc* bar; }; here so space isn't wasted for empty allocators? This would let you coalesce the allocator and non-allocator cases into a single case with a defaulted (empty) allocator.

95

#include<type_traits>

96

You probably want noexcept in the message instead of _NOEXCEPT.

98

This comment seems unnecessary in addition to the static_assert.

99

#include<utility>?

_Alloc __allocatorOnStack(_VSTD::move(__heapAllocator)); to avoid ADL on move, and because the Allocator requirements don't require *implicit* move construction. (I know, mega-nitpick.)

109

#include<new>

::new (static_cast<void*>(static_cast<char*>(__mem) + __paddedSize3)) _Alloc(__allocator); (to ensure True Placement New)

114

Allocator special member functions are forbidden to throw (http://eel.is/c++draft/allocator.requirements#tab:utilities.allocator.requirements)

118

deallocate is also forbidden to throw (same citation).

134

Ditto "static is implied but it's nice to be explicit"

_NOEXCEPT wouldn't hurt.

138

There's some "bonus" indentation on this line.

169

Style: reinterpret_cast<_Tp&>(__storage_).~_Tp(); saves a couple of valuable keystrokes. (And 172, 206, ... if you care)

186

::new (static_cast<void*>(&__storage_)) // ...

196

The parameters to is_convertible are the source expression's type-and-value-category, and the destination type - your arguments are reversed. Also, direct and copy-init can vary for class types in C++ so you should require is_constructible (I don't really care if it's "require instead" or "require as well"):

is_constructible<_Tp, _Value> && is_convertible<_Value, _Tp>

(The first argument of is_convertible and the non-first arguments to is_constructible go through declval, so X&& and X are equivalent.)

198

is_nothrow_constructible<_Tp, _Value> is equivalent and more concise.

199

Ditto ::new (static_cast<void*>(&__storage_) (I'll stop commenting these now.)

226

Too pretty; needs to be more __ugly (all three enumerators)

239

Weird indent.

252

Should be unsigned char so it can "provide storage" (http://eel.is/c++draft/intro.object#3)

alignas(_Tp) alignas(exception_ptr) would let you skip the __storageAlignment rigamarole

This class would be greatly simplified if this was an anonymous union: union { char __empty; _Tp __value; exception_ptr __exception; }; so the reinterpret_casting could go away.

263

Pure style comment: I'd use a default member initializer for __has_exception and not declare this constructor at all.

271

Ditto not a pointer to exception_ptr.

328

Ditto comments from the other specialization.

379

Why is this not simply:

template <>
class __task_promise<void> final : public __task_promise_base {
  using _Handle = coroutine_handle<__task_promise>;

public:
  _Handle get_return_object() _NOEXCEPT { return _Handle::from_promise(*this); }

  void return_void() _NOEXCEPT {}

  void unhandled_exception() _NOEXCEPT {
#ifndef _LIBCPP_NO_EXCEPTIONS
    __exception_ = current_exception();
#endif
  }

  void __lvalue_result() { __throw_if_exception(); }

  void __rvalue_result() { __throw_if_exception(); }

private:
  void __throw_if_exception() {
#ifndef _LIBCPP_NO_EXCEPTIONS
    if (__exception_) {
      rethrow_exception(__exception_);
    }
#endif
  }

#ifndef _LIBCPP_NO_EXCEPTIONS
  exception_ptr __exception_;
#endif
};

386

excess indent

415

#include<utility>?

431

Pure style comment: Let your destructor do the work:

task __old =  _VSTD::exchange(__coro_, _VSTD::exchange(__other.__coro_, {}));

531

Style: auto&& x is more concise.

Doesn't x dangle in this case? Be *really* explicit in the proposal about why it's a good idea if so.

559

@GorNishanov: Why isn't co_return foo; equivalent to co_return move(foo); for an automatic variable foo?

612

I suggest adding a test case with a stateful allocator.

[GorNishanov]

include/experimental/task

559

Not implemented yet in Clang and MSVC.
I filed a bug: https://bugs.llvm.org/show_bug.cgi?id=37265

[lewissbaker]

include/experimental/task

57

I meant to remove this _VSTD but missed it.
I went a bit overboard with _VSTD usage as I wasn't used to writing code within the std:: namespace ;)

I had missed that C++17 added the ability to add noexcept to the function type.
Nice! I'll add _NOEXCEPT in.

79

Should we also be adding overloads that take align_val_t for cases where the promise and/or coroutine frame is overaligned?

N4736 11.4.4(11) currently only mentions operator new overload with size_t argument.

@GorNishanov Are there plans to extend the allocator support for coroutines to use align_val_t overloads of operator new for over-aligned allocations?

81

I will run it through clang-format to fix up the formatting issues.

83

I don't think we can do that as we need to be able to lookup the _DeallocFunc* for a given coroutine frame regardless of the type of _Alloc, which is unknown in operator delete(). Placing the _DeallocFunc* member after the _Alloc would prevent that.

I could do something with if constexpr (is_empty_v<_Alloc>) to specialise operator new() for a stateless allocator.

Or maybe we can do something with the new-fangled [[no_unique_address]] attribute in C++20?

119

Do we need to use typename _Allocator::template rebind<char>::other as the allocator type rather than just using _Alloc directly or is it preferable to require the allocator to have a value_type of char (or should that be unsigned char)?

235

This approach will result in a different ABI for the task promise when compiled without exceptions.

@EricWF, @CaseyCarter: Does libc++ require standard library types to have the same ABI regardless of compiler flags?

379

This was done to avoid calling exception_ptr runtime functions in the common case where there is no exception created.

I thought I recalled this being done by someone from MS (yourself or perhaps Kenny Kerr?) in a generator<T> implementation to avoid the overhead of calls to runtime library functions for the exception_ptr default constructor, operator bool() and destructor as these are generally not inlinable. However, I currently can't find the reference code for this and it's possible I may be misremembering here.

A std::optional<exception_ptr> would achieve the same ends but I was conscious of incurring the extra dependency on the <optional> header.

I can simplify the implementation to what you suggest and re-apply the optimisation later if profiling determines it to be a performance problem.

384

This macro would seem to only enable nodiscard when building for c++2a or later.

Does libc++ not use [[nodiscard]] on any APIs when built against C++17?

396

The design I went for with cppcoro::task was to allow awaiting the value multiple times so that task<T> would act a bit like an asynchronous version of a lazy<T>.

If we only allowed co_await'ing the task once then this should:

• allow the coroutine frame to be destroyed earlier (eg. we could pass ownership of the coroutine frame to awaiter object which could destroy coroutine frame at end of statement rather than when ~task is run).
• eliminate the need for the conditional on await_ready().
• allow placing storage for the result in the awaiter object rather than the promise object - would this potentially provide more opportunity for copy elision?

431

I was considering implementing this by passing the parameter by value.

task& operator=(task __other) _NOEXCEPT {
  _VSTD::swap(__coro_, __other.__coro_);
}

436

I'm not sure either. The only place I've used ready() thus far has been in tests.

I'll remove them for now - we can always add them in later if we find a need.

451

Ok, I'll update this to an ASSERT.

531

This particular piece of code was just to check the expression's type was int&&.
The value would indeed dangle if this code was executed.
I did it this way as a workaround for the fact that co_await is not currently allowed inside a decltype() expression.

Also, auto&& has slightly different semantics to that of decltype(auto) when the initialising expression is a prvalue.

612

Will do.

[lewissbaker]

I have posted an updated version of the code here: https://gist.github.com/lewissbaker/38ba1d8a13e4fb0906559b7aa1c413d3

I'm still trying to figure out how to get the updated revision into this review.
I'm not familiar with Phabricator and it seems to want to create a new revision (review?) when I click the 'Update diff' button.
Does the updated diff need to be relative to the previous diff?
Any tips as to what I'm doing wrong are appreciated :)

[CaseyCarter]

In D46140#1083915, @lewissbaker wrote:

I have posted an updated version of the code here: https://gist.github.com/lewissbaker/38ba1d8a13e4fb0906559b7aa1c413d3

I'm still trying to figure out how to get the updated revision into this review.
I'm not familiar with Phabricator and it seems to want to create a new revision (review?) when I click the 'Update diff' button.
Does the updated diff need to be relative to the previous diff?
Any tips as to what I'm doing wrong are appreciated :)

Phabricator wants you to replace the entire differential when you update. Each diff should be relative to the original code, not to the previous diff.

[lewissbaker]

Addressed some feedback from initial version.

• factored out allocator padding calculations
• use anonymous unions instead of char-buffers and reinterpret_cast
• make co_await'ing an invalid task have undefined behaviour
• run clang-format over the file to fix some formatting

[GorNishanov]

include/experimental/task

372

Why I am concerned about

await_ready() => __coro.done()

is that check is inherently racy.

If the rule is, coroutine starts suspended and then you are allowed await on it once and that will start it. (Or you can use library helpers, spawn, when_all, etc. that will do similar thing. They will subscribe and start the coroutine).

Once the coroutine is started, you cannot touch it.

I struggle to see the use case.

Once coroutine is started, it has already has continuation hooked up and once finished it will resume the waiter that will lead to the destruction of the coroutine. So, nobody can check for done-ness.

If it has already been started, but has not finished, are we going to subscribe again? How would we distinguish the case that coroutine has not started, vs started but has not completed?

I see this await_ready() returning anything but false to be fraught with danger.

396

I played around some time ago and I think I was getting the best results when the result was in the promise.

We may get in the future a return_slot<T> type, that would allow to place result directly in the caller site (like RVO), if we get that, we would need the result in the promise, but, for now, it is fine to keep it there.

[lewissbaker]

include/experimental/task

131

Still need to qualify move() call with _VSTD::move() as per @CaseyCarter's earlier comment.

157

Add _NOEXCEPT.

372

I do not see this as being racy.

A task is not designed to be safe to access concurrently from multiple threads. I have tried to follow the general guideline of non-const methods cannot be called concurrently with any other access to the object, while const methods can generally be safely called concurrently with other const methods. The latest revision of task<T> does not have any const methods.

When you first call a task-returning coroutine it creates the coroutine initially suspended. When the task is first co_awaited this will call __coro.done() inside await_ready() (this is safe since the coroutine is suspended). The awaiting coroutine is then suspended and the task's coroutine is resumed. When the task's coroutine completes (ie. suspends at final_suspend point) it then becomes .done() and the awaiting coroutine is resumed.

If we treat the task as a normal non-thread-safe object then it is up to the caller to ensure the task is not accessed concurrently from another thread or coroutine. In this case, the awaiting coroutine should be the only coroutine that is accessing the task object for the duration of the co_await operation. As the awaiting coroutine will be suspended while the task's coroutine is executing, it will not have an opportunity to co_await the task again until the task completes, at which point the task's coroutine will be suspended at final_suspend-point and thus will be .done().

If a coroutine then subsequently co_awaits the task again after the first co_await operation has completed then it will see that the coroutine is alread .done() and so will continue without suspending.

How would we distinguish the case that coroutine has not started, vs started but has not completed?

A task object should only ever be able to observe the underlying coroutine being either suspended at initial_suspend (before the task is first co_awaited) or at final_suspend (after the first co_await has completed). The coroutine that has access to the task will be suspended while the task's coroutine is executing and so cannot observe the started-but-not-completed state.

Once coroutine is started, it has already has continuation hooked up and once finished it will resume the waiter that will lead to the destruction of the coroutine. So, nobody can check for done-ness.

The task's coroutine is not destroyed until ~task is executed, which may not be immediately after co_await expression resumes if the task object has been assigned to a local variable. In this case, the task's coroutine's .done() state would be observable.

I struggle to see the use case.

For one use-case, consider:

task<record> get_record(int id);

task<> example()
{
  // Want to only get the record once regardless of how many times
  // it's used below, but don't get the record if we don't need it.
  task<record> t = get_record(123);

  if (someCond)
  {
    record& r = co_await t;
    //...
  }

  //... later

  if (someOtherCond)
  {
    record& r = co_await t;
    //...
  }
}

372

As an aside, if we did want to allow multiple coroutines to concurrently co_await an asynchronous result then something like cppcoro::shared_task<T> would be more appropriate as it correctly handles synchronisation required to avoid the data races.

[CaseyCarter]

include/experimental/task

83

It occurred to me in the shower after making this comment that it would break because of the type erasure. I suspect the best choice would be to avoid storing an allocator when allocator_traits<_Alloc>::is_always_empty::value && is_default_constructible_v<_Alloc> is true, and just construct a new one in the deallocation function.

116

Is it intentional that this overload only takes lvalue parameters?

119

You need to go through allocator_traits for anything other than allocate, deallocate, and value_type. Your rebound allocator type would be typename allocator_traits<_Alloc>::template rebind_alloc<char>, for example.

I think I prefer always rebinding to the proper allocator type: it seems silly to make users specify exact allocator types when we can rebind them so easily.

131

_VSTD::move to avoid ADL hijacking (_Alloc is a program-defined type).

146

The allocator requirements mandate that the constructor does not throw; they don't mandate that it is declared noexcept.

150

::new to avoid broken garbage operator new overloads in _Alloc.

219

Ditto ::new.

379

That was Kenny's WinRT generator which I have since changed to use exception_ptr directly, and Microsoft's crappy implementation of exception_ptr. I have no idea about libc++'s exception_ptr. (I expect to fix ours when we can next break ABI: it's really terrible that everything isn't inlinable in the empty/fast case.)

392

Looking at these on my second readthrough I've realized you don't have a consistent SMF story for the promise hierarchy. Those classes are sometimes copy/movable depending on the parameter type and whether or not exceptions are enabled. Should they have explicitly deleted SMFs?

[GorNishanov]

include/experimental/task

372

With regard to the use case.

Let's imagine that we will get the return_slot<T> type in C++20.
Now, we don't have to store the result in the promise, return_value will directly constructed the result in the call site of the co_await.

Or, even without the return_slot<T> type, we can construct the result directly in the awaiter. (That is not what we are doing and it is fine, but, if it were the case, you would not be able to extract the result the second time.

It is similar to the std::future where you can only get the result once.

[GorNishanov]

include/experimental/task

402

Should this be

task& operator=(task &&__other) _NOEXCEPT {

[GorNishanov]

include/experimental/task

386

Do we really have a use case for implicit public constructor from coroutine_handle?
I think this should be a private constructor.

[GorNishanov]

include/experimental/task

411

since we have both

operator co_await () &
operator co_await() &&

is it the same as simply having a

operator co_await ()

You allow mutable l-values, R-values, but not const values.

Also, why operator co_await at all? as opposed to three await_xxx functions?

I am thinking specifying the wording that allow implementators flexibility to pick either approach.
Is there a reason to mandate one over the other?

[lewissbaker]

include/experimental/task

83

I assume you mean allocator_traits<_Alloc>::is_always_equal?

116

Yes, the wording from coroutines TS states that operator new is passed lvalue-references to each of the coroutine parameters.

11.4.4(7) ... If the lookup finds an allocation function in the scope of P, overload resolution is performed on a function call created by assembling an argument list. The first argument is the amount of space requested, and has type std::size_t. The lvalues p1 ... pn are the succeeding arguments. ...

372

It is similar to the std::future where you can only get the result once.

Yes, I guess that single-await or multi-await is really the major choice here.

If we limit it so that you can only co_await the task once then it does give more flexibility on implementation that could potentially lead to a more efficient implementation in some cases. This does prevent the use-case I mentioned above, however.

I'm open to putting this restriction in for now - it matches the behaviour of std::future::get().

If we do this then we might want to consider having a single operator co_await that returns a prvalue - although this would potentially incur an extra copy/move in some cases compared with returning a reference to the value stored in the promise.

Let's imagine that we will get the return_slot<T> type in C++20.

Can P0878R0 (sub-object copy elision) help here if we put the result object in the awaiter and try to return the sub-object from await_resume()? If this can be made to work we may not need return_slot<T> to get RVO for tasks.

386

Yes, it probably should be a private constructor.

It was added to allow the coroutine to implicitly construct the task from the coroutine_handle returned from get_return_object() - this simplifies the implementation but isn't required.

I'll change get_return_object() to return a task<T> instead and add some appropriate friend-declarations.

392

The promise types are not really part of the public API so I wasn't so concerned with locking down the interface.

I can add the explicit declaration of deleted SMFs if you like.

402

Yes, the parameter could be made task&&.
I'd just need to move-construct to a temporary inside the body instead.

411

The reason operator co_await() are added is to allow overloading so that when a task<T>& is awaited it returns a T& and when a task<T>&& is awaited it returns a T&&`.

We can't overload await_resume() & and await_resume() && since the co_await mechanics states that it always calls await_resume() on the awaiter lvalue.

If we change task<T> to only support awaiting once then we could get rid of operator co_await() and just add await_xxx() methods directly on task<T>.

Having said that, it does feel a little dirty having the await_xxx() methods popup in intellisense completion when auto-completing methods on a task<T> instance - these methods are not intended to be called directly by users of the type.

[lewissbaker]

• Explicitly delete __task_promise_base special member functions.
• Declare _task_promise_base::operator delete() as _NOEXCEPT.
• Explicitly scope _VSTD::move() calls for user-defined types.
• Explicitly call global-scope placement operator new for user-defined types.
• Tweak task::operator=() to remove ambiguity with deleted operator=(const task&).

[CaseyCarter]

include/experimental/task

83

Yes, I did. I was thinking "use is_always_equal instead of is_empty" and apparently my brain decided to type them both.

[GorNishanov]

include/experimental/task

402

Ah Ok. No need to change it then. I am starting to work on wording and trying to figure out which things are the artifacts of the implementation and which should be carried over into the wording.

411

I will try to specify the wording so that both await_xxx and a single operator co_await are acceptable implementations.

I do strongly feel that task<T> should have await once semantics. It has a potential to unlock RVO. It is (potentially) less prone to misuse (my subjective feeling).

await_resume() for task<T> should return T. It allows RVO and eliminates dangling references.

No need to change anything at the moment in the implementation. Let's see how specification works out and then alter implementation (if needed) to match. Sometimes specifying something precisely gives clarity to a problem :-).

[EricWF]

Jeez, this is weird. Having other STL maintainers review additions to libc++ :-)

[CaseyCarter]

In D46140#1086987, @EricWF wrote:

Jeez, this is weird. Having other STL maintainers review additions to libc++ :-)

Speaking of which, I don't know *your* policy but we std`-qualify pretty calls even when they can't be ADL hijacked like _VSTD::terminate(); or _VSTD::move(reference_to_internal_non_template_type). There are a few such in here that I didn't comment.

[EricWF]

My apologies to the reviewers with knowledge of libc++'s existing style, but I would like to modernize the style with new headers like this, which already use C++14 features, like auto return types. That is, I would rather use constexpr over _LIBCPP_CONSTEXPR, and noexcept over _NOEXCEPT, ect. At this point I think it's safe to no longer a need for C++11, and in this header C++14, features to be guarded using macros.

Does everyone agree it's reasonable to limit this header to C++14 and beyond? If so, let's #ifdef out the header contents in older dialects.
If we bump this up to C++17 we can use variant to handle the non-trivially destructible union.

For whitespace, feel free to just clang-format the header.

Also, tests! Let me know if you need any help building and setting up a testing environment. Take a look at the Building Libc++ and Testing Libc++ docs to get started.

include/experimental/task

58

Let's reuse __aligned_allocation_size from memory_resource. I would move it into __memory, drop the assert, and make it constexpr

326

We'll want a constant ABI here. The exception_ptr type should be complete even when exceptions are disabled, so lets just always enable this.

include/module.modulemap

507

By convention this should be it's own module.

[EricWF]

include/experimental/task

193

= default?

280

= default ?

[lewissbaker]

Many improvements:

• Remove move-assignment operator
• task<T>::operator co_await() now returns by value when awaiting task<T> rvalue.
• Explicitly scope calls to std-library functions that live in _VSTD.
• Make __task_promise ABI stable under exception/no-exception compilation modes.
• Moved __aligned_allocation_size to <experimental/__memory>
• Make task<T> have a default template parameter of void, allowing task<> to mean task<void>
• Allow co_return { ... } by adding overload of return_value() taking T&& and another taking initializer_list<U>.
• Implement optimisation that avoids storing the allocator in the coroutine frame if allocator_traits<A>::is_always_equal is true and the allocator is default constructible.
• Added unit-tests (currently failing some custom allocator tests under debug builds)

[lewissbaker]

• Make task<T> constructor from coroutine_handle private.
• Run clang-format over <experimental/task>.

[GorNishanov]

LGTM

With a few suggestions.

1. Remove swap. It is not part of the proposal at the moment.
2. If you feel like, add a lazy<T> alias to task<T>, that way we can taste both names and see how it feels.
3. Also, there is still an outstanding work to add handling of implicit 'this'. Probably would be good to add before committing,

My apologies for taking so long to review.

[lewissbaker]

test/std/experimental/task/sync_wait.hpp

36–37 ↗

(On Diff #156641)

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.

[lewissbaker]

• Don't use __allocator as variable name (it is #defined to NASTY_MACRO).
• Remove workaround in test for https://bugs.llvm.org/show_bug.cgi?id=37265
• Add support for custom allocators on member functions.

[lewissbaker]

Fix race condition in __oneshot_event::set() method used by sync_wait().

[modocache]

Committed in rL357010. Apologies, I forgot to include the differential revision in the commit message so this diff wasn't closed automatically as a result. I'll comment on rL357010 with the missing information.

[lewissbaker]

Reopening as the commit for this diff was reverted due to it breaking the buildbot module builds.

[lewissbaker]

Added missing 'require coroutines' for experimental/task entry in module.modulemap
Added missing #include in experimental/task header that was failing module builds

[lewissbaker]

@EricWF This implementation will currently not work with MSVC as MSVC does not yet support the symmetric-transfer capability added to the coroutines specification in P0913R0.

Is MSVC a target that libc++ needs to support coroutines with?
Or can we just say the task<T> class is unsupported for compilers that don't support P0913?

I have added the 'requires coroutines' line to the task modulemap entry which should hopefully fix the module build failures this diff was seeing.
Are there any other potential gotchas that I should make sure I test?

test/std/experimental/task/task.basic/task_of_value.pass.cpp

25 ↗

(On Diff #192384)

@EricWF This line may fail to compile on older versions of clang without the coroutines bugfix for https://bugs.llvm.org/show_bug.cgi?id=37265.

Should I just change this back to std::move(p) here, since the intention is to test the library, not the compiler?
Alternatively I can conditionally compile this line as either co_return p; or co_return std::move(p); depending on the compiler version.

[CaseyCarter]

include/experimental/__memory

80 ↗

(On Diff #192208)

This is missing preconditions that __a is a power of 2, and that __s <= -__a.

include/experimental/task

29

"requires a compiler with support for coroutines" would be more informative.

142

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.

211

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)

221

This mem-initializer for __state_ is redundant with the default member initializer on 306.

228

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.

245

Are you certain that unhandled_exception can't possibly be called after storing a value? If so, this would leak the value.

255

Style: you've been using the _v variable templates for traits elsewhere.

262

Unnecessary std:: qualifier. (Occurs repeatedly.)

264

Style: the use of trailing-return-type SFINAE here is inconsistent with the use of template parameter SFINAE on 254.

309

These __empty_ members seem extraneous.

310

Should we static_assert that _Tp is a destructible object type?

374

Do we care about task<cv-void>?

390

Should these __foovalue_result members be foo-qualified to make them harder to misuse?

408

Similar to above, should we static_assert that _Tp is void, an lvalue reference type, or a destructible non-array object type?

454

this-> is extraneous in these classes that derive from the concrete _AwaiterBase. (And on 470)

459

Is missing a subject. How about "co_await on an invalid task<T> has undefined behavior"? (And on 475)

test/std/experimental/task/awaitable_traits.hpp

17 ↗

(On Diff #192384)

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.)

test/std/experimental/task/counted.hpp

81 ↗

(On Diff #192384)

IIUC that the tests all require C++17, you could declare these inline and avoid the external definitions / DEFINE_COUNTED_VARIABLES macro.

90 ↗

(On Diff #192384)

Should nextId_ be initialized to 1, as it is in reset on 40?

test/std/experimental/task/manual_reset_event.hpp

18 ↗

(On Diff #192384)

"currently". Also, "was not currently" is weird.

23 ↗

(On Diff #192384)

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.

25 ↗

(On Diff #192384)

Ditto "using reserved names in test code". (Occurs repeatedly.)

40 ↗

(On Diff #192384)

_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.

47 ↗

(On Diff #192384)

Why are we concerned about data races on __event_->__state_, but not __event_->__awaitingCoroutine_?

49 ↗

(On Diff #192384)

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.)

86 ↗

(On Diff #192384)

thread

88 ↗

(On Diff #192384)

"awaits the" what? The suspense is killing me!

92 ↗

(On Diff #192384)

std::exchange

102 ↗

(On Diff #192384)

What's the antecedent of "it"?

test/std/experimental/task/sync_wait.hpp

14 ↗

(On Diff #192384)

There's no such header in the working draft or the Coroutines TS.

70 ↗

(On Diff #192384)

Ditto "doesn't do what you think it does and is unnecessary."

115 ↗

(On Diff #192384)

Ditto "not in test code". You should include "test_macros.h" and use TEST_NO_EXCEPTIONS. (Occurs repeatedly.)

135 ↗

(On Diff #192384)

#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.)

139 ↗

(On Diff #192384)

Here's another _VSTD that should be std. Please audit all test code.

36–37 ↗

(On Diff #156641)

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.)

test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp

18 ↗

(On Diff #192384)

This is not a Standard or Coroutines TS header.

26 ↗

(On Diff #192384)

static is extraneous in an anonymous namespace.

37 ↗

(On Diff #192384)

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.

54 ↗

(On Diff #192384)

This is almost certainly going to cause problems, since it leaves the moved-from allocator invalid. (And on 68.)

75 ↗

(On Diff #192384)

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.

78 ↗

(On Diff #192384)

allocate needs to return T*. (Also, std::size_t.) (And on 88)

90 ↗

(On Diff #192384)

Here again, I suggest an assertion that we're not freeing more bytes than have been allocated.

221 ↗

(On Diff #192384)

In libc++ tests main is always declared int main(int, char**) (and explicitly returns 0) to support the freestanding work. (Occurs repeatedly.)

test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp

46 ↗

(On Diff #192384)

Again, leaving me in suspense.

[lewissbaker]

Thanks very much for the detailed review @CaseyCarter! Very much appreciated :)

include/experimental/__memory

80 ↗

(On Diff #192208)

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.

include/experimental/task

29

This messaging was copied from <experimental/coroutine>.

I'll update the message there as well.

142

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?

221

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.

245

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.

309

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.

310

Sure, will do.

I wonder if we should make this a requirement in the wording of P1056?
What do you think @GorNishanov?

374

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?

390

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();

408

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.

454

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?

test/std/experimental/task/awaitable_traits.hpp

17 ↗

(On Diff #192384)

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.

Similarly for sync_wait() which is proposed in 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.

test/std/experimental/task/counted.hpp

90 ↗

(On Diff #192384)

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.

test/std/experimental/task/manual_reset_event.hpp

47 ↗

(On Diff #192384)

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_.

test/std/experimental/task/sync_wait.hpp

36–37 ↗

(On Diff #156641)

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

test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp

18 ↗

(On Diff #192384)

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)?

54 ↗

(On Diff #192384)

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.

[lewissbaker]

This updated diff should address most of @CaseyCarter's review comments.

For detailed changelog you can find individual changes here https://github.com/lewissbaker/libcxx/commits/coro_task

There are a few outstanding issues that still need some clarification on how to proceed:

• Should the task tests be taking a dependency on <experimental/memory_resource>?
• How should I implement a precondition check for __aligned_allocation_size when it is declared constexpr?
• I've not yet implemented support for task<cv-void>. Is this something we should support?
• I've not yet handling allocators with fancy pointer types. If we are to support them is there a standard way to convert a fancy pointer to a void* to return from promise_type::operator new()? Does it even make sense to support them if the compiler is potentially going to be storing raw pointers to things inside the coroutine frame?

[lewissbaker]

include/experimental/task

161

This should use explicit move construction rather than implicit move construction.

[lewissbaker]

Gentle ping.

Is there anything else people would like to see changed?

include/experimental/task

+// -*- C++ -*-
+//===----------------------------- coroutine -----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP_EXPERIMENTAL_TASK
+#define _LIBCPP_EXPERIMENTAL_TASK
+
+#include <experimental/__config>
+#include <experimental/coroutine>
+
+#include <exception>
+#include <utility>
+#include <type_traits>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+#ifdef _LIBCPP_HAS_NO_COROUTINES
+#if defined(_LIBCPP_WARNING)
+_LIBCPP_WARNING("<experimental/task> cannot be used with this compiler")
+#else
+#warning <experimental/task> cannot be used with this compiler
+#endif
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_COROUTINES
+
+////// task<T>
+
+struct __task_promise_final_awaitable {
+  _LIBCPP_INLINE_VISIBILITY
+  _LIBCPP_CONSTEXPR bool await_ready() const _NOEXCEPT { return false; }
+
+  template <typename _TaskPromise>
+  _LIBCPP_INLINE_VISIBILITY coroutine_handle<>
+  await_suspend(coroutine_handle<_TaskPromise> __coro) const _NOEXCEPT {
+    _LIBCPP_ASSERT(
+        __coro.promise().__continuation_,
+        "Coroutine completed without a valid continuation attached.");
+    return __coro.promise().__continuation_;
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void await_resume() const _NOEXCEPT {}
+};
+
+class _LIBCPP_TYPE_VIS __task_promise_base {
+  using _DeallocFunc = void(void* __ptr, size_t __size) _NOEXCEPT;
+
+  static _LIBCPP_CONSTEXPR size_t __pad(size_t __value,
+                                        size_t __alignment) _NOEXCEPT {
+    return (__value + (__alignment - 1u)) & ~(__alignment - 1u);
+  }
+
+  static _LIBCPP_CONSTEXPR size_t
+  __get_dealloc_func_offset(size_t __frameSize) _NOEXCEPT {
+    return __pad(__frameSize, alignof(_DeallocFunc*));
+  }
+
+  static _LIBCPP_CONSTEXPR size_t
+  __get_padded_frame_size(size_t __frameSize) _NOEXCEPT {
+    return __get_dealloc_func_offset(__frameSize) + sizeof(_DeallocFunc*);
+  }
+
+  template <typename _Alloc>
+  static _LIBCPP_CONSTEXPR size_t
+  __get_allocator_offset(size_t __frameSize) _NOEXCEPT {
+    return __pad(__get_padded_frame_size(__frameSize), alignof(_Alloc));
+  }
+
+  template <typename _Alloc>
+  static _LIBCPP_CONSTEXPR size_t
+  __get_padded_frame_size_with_allocator(size_t __frameSize) _NOEXCEPT {
+    return __get_allocator_offset<_Alloc>(__frameSize) + sizeof(_Alloc);
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  static _DeallocFunc*& __get_dealloc_func(void* __frameStart,
+                                           size_t __frameSize) _NOEXCEPT {
+    return *reinterpret_cast<_DeallocFunc**>(
+        static_cast<char*>(__frameStart) +
+        __get_dealloc_func_offset(__frameSize));
+  }
+
+  template <typename _Alloc>
+  _LIBCPP_INLINE_VISIBILITY static _Alloc&
+  __get_allocator(void* __frameStart, size_t __frameSize) _NOEXCEPT {
+    return *reinterpret_cast<_Alloc*>(
+        static_cast<char*>(__frameStart) +
+        __get_allocator_offset<_Alloc>(__frameSize));
+  }
+
+public:
+  static void* operator new(size_t __size) {
+    // Allocate space for an extra pointer immediately after __size that holds
+    // the type-erased deallocation function.
+    void* __pointer = ::operator new(__get_padded_frame_size(__size));
+
+    _DeallocFunc*& __deallocFunc = __get_dealloc_func(__pointer, __size);
+    __deallocFunc = [](void* __pointer, size_t __size) _NOEXCEPT {
+      ::operator delete(__pointer, __get_padded_frame_size(__size));
+    };
+
+    return __pointer;
+  }
+
+  template <typename _Alloc, typename... _Args>
+  static void* operator new(size_t __size, allocator_arg_t, _Alloc& __allocator,
+                            _Args&...) {
+    static_assert(is_same<typename _Alloc::value_type, char>::value,
+                  "task<T> coroutine custom allocator must have a 'value_type' "
+                  "of 'char'.");
+
+    void* __pointer = __allocator.allocate(
+        __get_padded_frame_size_with_allocator<_Alloc>(__size));
+
+    _DeallocFunc*& __deallocFunc = __get_dealloc_func(__pointer, __size);
+    __deallocFunc = [](void* __pointer, size_t __size) _NOEXCEPT {
+      static_assert(is_nothrow_move_constructible<_Alloc>::value,
+                    "task<T> coroutine custom allocator requires a noexcept "
+                    "move constructor");
+
+      _Alloc& __allocatorInFrame = __get_allocator<_Alloc>(__pointer, __size);
+      _Alloc __allocatorOnStack = move(__allocatorInFrame);
+      __allocatorInFrame.~_Alloc();
+
+      size_t __paddedSize =
+          __get_padded_frame_size_with_allocator<_Alloc>(__size);
+
+      // Allocator requirements state that deallocate() must not throw.
+      // See [allocator.requirements] from C++ standard.
+      // We are relying on that here.
+      __allocatorOnStack.deallocate(static_cast<char*>(__pointer),
+                                    __paddedSize);
+    };
+
+    // Copy the allocator into the heap frame.
+
+    static_assert(is_nothrow_copy_constructible<_Alloc>::value,
+                  "task<T> coroutine custom allocator requires a noexcept copy "
+                  "constructor");
+
+    new (static_cast<void*>(_VSTD::addressof(
+        __get_allocator<_Alloc>(__pointer, __size)))) _Alloc(__allocator);
+
+    return __pointer;
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  static void operator delete(void* __pointer, size_t __size) {
+    __get_dealloc_func(__pointer, __size)(__pointer, __size);
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  suspend_always initial_suspend() const _NOEXCEPT { return {}; }
+
+  _LIBCPP_INLINE_VISIBILITY
+  __task_promise_final_awaitable final_suspend() _NOEXCEPT { return {}; }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __set_continuation(coroutine_handle<> __continuation) {
+    _LIBCPP_ASSERT(!__continuation_, "task already has a continuation");
+    __continuation_ = __continuation;
+  }
+
+private:
+  friend class __task_promise_final_awaitable;
+
+  coroutine_handle<> __continuation_;
+};
+
+template <typename _Tp>
+class _LIBCPP_TEMPLATE_VIS __task_promise final : public __task_promise_base {
+  using _Handle = coroutine_handle<__task_promise>;
+
+public:
+  __task_promise() _NOEXCEPT {}
+
+  ~__task_promise() {
+    switch (__state_) {
+    case _State::__value:
+      __value_.~_Tp();
+      break;
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    case _State::__exception:
+      __exception_.~exception_ptr();
+      break;
+#endif
+    case _State::__no_value:
+      break;
+    };
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  _Handle get_return_object() _NOEXCEPT { return _Handle::from_promise(*this); }
+
+  void unhandled_exception() _NOEXCEPT {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    new (static_cast<void*>(&__exception_)) exception_ptr(current_exception());
+    __state_ = _State::__exception;
+#else
+    _LIBCPP_ASSERT(
+        false, "task<T> coroutine unexpectedly called unhandled_exception()");
+#endif
+  }
+
+  // Only enable return_value() overload if _Tp is implicitly constructible from _Value
+  template <typename _Value,
+            enable_if_t<is_convertible<_Value, _Tp>::value, int> = 0>
+  void return_value(_Value&& __value)
+      _NOEXCEPT_((is_nothrow_constructible<_Tp, _Value>::value)) {
+    new (static_cast<void*>(_VSTD::addressof(__value_)))
+        _Tp(static_cast<_Value&&>(__value));
+
+    // Only set __state_ after successfully constructing the value.
+    // If constructor throws then state will be updated by unhandled_exception().
+    __state_ = _State::__value;
+  }
+
+  _Tp& __lvalue_result() {
+    __throw_if_exception();
+    return __value_;
+  }
+
+  _Tp&& __rvalue_result() {
+    __throw_if_exception();
+    return static_cast<_Tp&&>(__value_);
+  }
+
+private:
+  void __throw_if_exception() {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    if (__state_ == _State::__exception) {
+      rethrow_exception(__exception_);
+    }
+#endif
+  }
+
+  enum class _State {
+    __no_value,
+    __value
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    ,
+    __exception
+#endif
+  };
+
+  _State __state_ = _State::__no_value;
+  union {
+    char __empty_;
+    _Tp __value_;
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    exception_ptr __exception_;
+#endif
+  };
+};
+
+template <typename _Tp>
+class __task_promise<_Tp&> final : public __task_promise_base {
+  using _Ptr = _Tp*;
+  using _Handle = coroutine_handle<__task_promise>;
+
+public:
+  __task_promise() _NOEXCEPT {}
+
+  ~__task_promise() {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    if (__has_exception_) {
+      __exception_.~exception_ptr();
+    }
+#endif
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  _Handle get_return_object() _NOEXCEPT { return _Handle::from_promise(*this); }
+
+  void unhandled_exception() _NOEXCEPT {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    new (static_cast<void*>(&__exception_)) exception_ptr(current_exception());
+    __has_exception_ = true;
+#else
+    _LIBCPP_ASSERT(
+        false, "task<T> coroutine unexpectedly called unhandled_exception()");
+#endif
+  }
+
+  void return_value(_Tp& __value) _NOEXCEPT {
+    new (static_cast<void*>(&__pointer_)) _Ptr(_VSTD::addressof(__value));
+  }
+
+  _Tp& __lvalue_result() {
+    __throw_if_exception();
+    return *__pointer_;
+  }
+
+  _Tp& __rvalue_result() { return __lvalue_result(); }
+
+private:
+  void __throw_if_exception() {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    if (__has_exception_) {
+      rethrow_exception(__exception_);
+    }
+#endif
+  }
+
+  union {
+    char __empty_;
+    _Ptr __pointer_;
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    exception_ptr __exception_;
+#endif
+  };
+#ifndef _LIBCPP_NO_EXCEPTIONS
+  bool __has_exception_ = false;
+#endif
+};
+
+template <>
+class __task_promise<void> final : public __task_promise_base {
+  using _Handle = coroutine_handle<__task_promise>;
+
+public:
+  _Handle get_return_object() _NOEXCEPT { return _Handle::from_promise(*this); }
+
+  void return_void() _NOEXCEPT {}
+
+  void unhandled_exception() _NOEXCEPT {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    __exception_ = current_exception();
+#endif
+  }
+
+  void __lvalue_result() { __throw_if_exception(); }
+
+  void __rvalue_result() { __throw_if_exception(); }
+
+private:
+  void __throw_if_exception() {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    if (__exception_) {
+      rethrow_exception(__exception_);
+    }
+#endif
+  }
+
+#ifndef _LIBCPP_NO_EXCEPTIONS
+  exception_ptr __exception_;
+#endif
+};
+
+template <typename _Tp>
+class _LIBCPP_TEMPLATE_VIS _LIBCPP_NODISCARD_AFTER_CXX17 task {
+public:
+  using promise_type = __task_promise<_Tp>;
+
+private:
+  using _Handle = coroutine_handle<__task_promise<_Tp> >;
+
+  class _AwaiterBase {
+  public:
+    _AwaiterBase(_Handle __coro) _NOEXCEPT : __coro_(__coro) {}
+
+    _LIBCPP_INLINE_VISIBILITY
+    bool await_ready() const { return __coro_.done(); }
+
+    _LIBCPP_INLINE_VISIBILITY
+    _Handle await_suspend(coroutine_handle<> __continuation) const {
+      __coro_.promise().__set_continuation(__continuation);
+      return __coro_;
+    }
+
+  protected:
+    _Handle __coro_;
+  };
+
+public:
+  _LIBCPP_INLINE_VISIBILITY
+  task(_Handle __coro) _NOEXCEPT : __coro_(__coro) {}
+
+  _LIBCPP_INLINE_VISIBILITY
+  task(task&& __other) _NOEXCEPT
+      : __coro_(_VSTD::exchange(__other.__coro_, {})) {}
+
+  task(const task&) = delete;
+  task& operator=(const task&) = delete;
+
+  _LIBCPP_INLINE_VISIBILITY
+  ~task() {
+    if (__coro_)
+      __coro_.destroy();
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  task& operator=(task __other) _NOEXCEPT {
+    swap(__other);
+    return *this;
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void swap(task& __other) _NOEXCEPT { _VSTD::swap(__coro_, __other.__coro_); }
+
+  _LIBCPP_INLINE_VISIBILITY
+  auto operator co_await() & {
+    class _Awaiter : public _AwaiterBase {
+    public:
+      using _AwaiterBase::_AwaiterBase;
+
+      _LIBCPP_INLINE_VISIBILITY
+      decltype(auto) await_resume() {
+        return this->__coro_.promise().__lvalue_result();
+      }
+    };
+
+    _LIBCPP_ASSERT(__coro_,
+                   "Undefined behaviour to co_await an invalid task<T>");
+    return _Awaiter{__coro_};
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  auto operator co_await() && {
+    class _Awaiter : public _AwaiterBase {
+    public:
+      using _AwaiterBase::_AwaiterBase;
+
+      _LIBCPP_INLINE_VISIBILITY
+      decltype(auto) await_resume() {
+        return this->__coro_.promise().__rvalue_result();
+      }
+    };
+
+    _LIBCPP_ASSERT(__coro_,
+                   "Undefined behaviour to co_await an invalid task<T>");
+    return _Awaiter{__coro_};
+  }
+
+private:
+  _Handle __coro_;
+};
+
+_LIBCPP_END_NAMESPACE_EXPERIMENTAL_COROUTINES
+
+#endif
+
+////////////////////////
+// Tests for task<T>
+
+using namespace std::experimental;
+
+task<void> f1() { co_return; }
+
+task<void> f2() { co_await f1(); }
+
+task<void> f3() {
+  task<void> t = f1();
+  co_await t;
+}
+
+task<int> g1() { co_return 123; }
+
+task<int> g2() { co_return co_await g1() + 100; }
+
+task<int> g3() {
+  // co_await rvalue returns rvalue reference to result.
+  {
+    // BUG: This will leave x as a dangling rvalue reference.
+    // This code is not intended to be executed, it's just here
+    // to check the return type of co_await'ing a task<T>&&.
+    decltype(auto) x = co_await g1();
+    static_assert(std::is_same<decltype(x), int&&>::value);
+  }
+
+  // co_await lvalue returns lvalue reference to result
+  task<int> t = g1();
+  decltype(auto) y = co_await t;
+  static_assert(std::is_same_v<decltype(y), int&>);
+
+  co_return y * 2;
+}
+
+class MoveOnly {
+public:
+  MoveOnly();
+  MoveOnly(const MoveOnly&) = delete;
+  MoveOnly(MoveOnly&&) noexcept;
+  ~MoveOnly();
+  int get() const;
+
+private:
+  void* _data;
+};
+
+task<MoveOnly> h1(bool x) {
+  if (x) {
+    MoveOnly value;
+    co_return std::move(value);
+  }
+  co_return MoveOnly{};
+}
+
+task<int> h2() {
+  auto x = co_await h1(true);
+  co_return x.get();
+}
+
+static int x;
+static int y;
+
+task<int&> h3(bool cond) { co_return cond ? x : y; }
+
+task<void> h3consumer() {
+  int& result = co_await h3(true);
+  result = 32;
+}
+
+// custom allocator tests
+
+template <typename Allocator>
+task<void> a1(std::allocator_arg_t, Allocator allocator, bool x) {
+  co_return;
+}
+
+class my_allocator {
+public:
+  using value_type = char;
+  my_allocator();
+  my_allocator(const my_allocator&) noexcept;
+  my_allocator(my_allocator&&) noexcept;
+  char* allocate(size_t n);
+  void deallocate(char* p, size_t n);
+
+private:
+  void* state;
+};
+
+task<void> a2() {
+  my_allocator alloc;
+  a1(std::allocator_arg, alloc, true);
+  co_return;
+}
+
+task<void> a2a() {
+  my_allocator alloc;
+  return a1(std::allocator_arg, alloc, true);
+}
+
+task<void> a3() {
+  co_await a1(std::allocator_arg, std::allocator<char>{}, false);
+}
+
+task<void> a3a() {
+  return a1(std::allocator_arg, std::allocator<char>{}, false);
+}
 No newline at end of file

include/module.modulemap

module experimental {
 
     module algorithm {
       header "experimental/algorithm"
       export *
     }
      module coroutine {
       requires coroutines
       header "experimental/coroutine"
+      header "experimental/task"
       export *
     }
     module deque {
       header "experimental/deque"
       export *
     }
     module dynarray {
       header "experimental/dynarray"