[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked an inline comment as done.
lewissbaker added a comment.

Gentle ping.

Is there anything else people would like to see changed?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked 3 inline comments as done.
lewissbaker added inline comments.



Comment at: include/experimental/task:160
+__get_allocator<_CharAlloc>(__pointer, __size);
+_CharAlloc __allocatorOnStack = _VSTD::move(__allocatorInFrame);
+__allocatorInFrame.~_CharAlloc();

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


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 192944.
lewissbaker marked 6 inline comments as done.
lewissbaker added a comment.

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 
?
- How should I implement a precondition check for `__aligned_allocation_size` 
when it is declared constexpr?
- I've not yet implemented support for `task`. 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?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/coroutine
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,157 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = ::sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = ::sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+struct my_error {};
+
+struct throws_on_destruction
+{
+  ~throws_on_destruction() noexcept(false)
+  {
+throw my_error{};
+  }
+};
+
+void test_uncaught_exception_thrown_after_co_return()
+{
+  counted::reset();
+
+  assert(counted::active_instance_count() == 0);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() == 0);
+
+  {
+auto t = []() -> std::experimental::task
+{
+  throws_on_destruction d;
+  co_return counted{};
+}();
+
+try {
+  (void)::sync_wait(std::move(t));
+  assert(false);
+} catch (const my_error&) {
+}
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

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 `` was not 
marked constexpr and so was able to use `_LIBCPP_ASSERT`.



Comment at: include/experimental/task:28
+#if defined(_LIBCPP_WARNING)
+_LIBCPP_WARNING(" 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 .

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 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 final : public __task_promise_base {
+  using _Handle = coroutine_handle<__task_promise>;

CaseyCarter wrote:
> Do we care about `task`?
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 

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Casey Carter via Phabricator via cfe-commits]

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::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 ::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 
+  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 final : public __task_promise_base {
+  using _Handle = coroutine_handle<__task_promise>;

Do we care about `task`?



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");
+return _Awaiter{__coro_};

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



Comment at: include/experimental/task:28
+#if defined(_LIBCPP_WARNING)
+_LIBCPP_WARNING(" cannot be 

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked an inline comment as done.
lewissbaker added a comment.

@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` 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?




Comment at: test/std/experimental/task/task.basic/task_of_value.pass.cpp:25
+   auto p = std::make_unique(123);
+   co_return p; // Should be implicit std::move(p) here.
+ }

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


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 192384.
lewissbaker added a comment.

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


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,86 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+int main()
+{
+  test_return_value_lifetime();
+  return 0;
+}
Index: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
@@ -0,0 +1,57 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_parameter_lifetime()
+{
+  counted::reset();
+
+  auto f = [](counted c) -> std::experimental::task
+  {
+co_return c.id();
+  };
+
+  {
+auto t = f({});
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() <= 2); // Ideally <= 1
+
+auto id = sync_wait(t);
+assert(id == 1);
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+
+// We are relying on C++17 copy-elision when passing the temporary counter
+// into f(). Then f() must move the parameter into the coroutine 

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker reopened this revision.
lewissbaker added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Brian Gesiak via Phabricator via cfe-commits]

modocache closed this revision.
modocache added a comment.

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.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits