Page MenuHomePhabricator

[coroutines] Add noop_coroutine to <experimental/coroutine>
ClosedPublic

Authored by GorNishanov on Mar 31 2018, 7:20 AM.

Details

Summary

A recent addition to Coroutines TS (https://wg21.link/p0913) adds a pre-defined
coroutine noop_coroutine that does nothing.

This patch implements require library types in <experimental/coroutine>

Related clang and llvm patches:

https://reviews.llvm.org/D45114
https://reviews.llvm.org/D45120

Diff Detail

Event Timeline

@EricWF , gentle ping. Super tini-tiny change. Last piece missing to be post-Jax 2018 compilant

lewissbaker accepted this revision.Apr 3 2018, 6:13 PM

The coroutine_handle<noop_coroutine_promise> type does not have a from_address() or a from_promise() static functions in the same way that the coroutine_handle<P> implementation does.
Is this intentional or an oversight in the TS wording?

They don't seem hugely useful, so I'm not that worried.
If you know that you have a coroutine_handle<noop_coroutine_promise> then you can just use noop_coroutine() to get the handle instead.

test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
62

Maybe also add a test for from_address()?
eg. assert(coroutine_handle<>::from_address(h.address()) == base);

This revision is now accepted and ready to land.Apr 3 2018, 6:13 PM
EricWF added inline comments.Apr 3 2018, 9:31 PM
include/experimental/coroutine
262

This whole thing should be wrapped in a __has_builtin(__builtin_coro_noop) so the header still compiles with older clang versions.

294

This should just be _LIBCPP_INLINE_VISIBILITY. We try not to use _LIBCPP_ALWAYS_INLINE in new code.

test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
12

This probably needs an // XFAIL: clang-5, clang-6

The coroutine_handle<noop_coroutine_promise> type does not have a from_address() or a from_promise() static functions in the same way that the coroutine_handle<P> implementation does.
Is this intentional or an oversight in the TS wording?

They don't seem hugely useful, so I'm not that worried.
If you know that you have a coroutine_handle<noop_coroutine_promise> then you can just use noop_coroutine() to get the handle instead.

This is intentional. The only way to get a noop coroutine is to ask for it via noop_coroutine()

incorporated review feedback

GorNishanov marked 4 inline comments as done.Apr 3 2018, 10:25 PM
lewissbaker added inline comments.Apr 3 2018, 11:57 PM
include/experimental/coroutine
294

Should the same change be applied to the other usages of _LIBCPP_ALWAYS_INLINE in this file?
Should some of them be marked constexpr to be consistent with noop_coroutine_handle member functions above?

EricWF added inline comments.Apr 4 2018, 2:12 AM
include/experimental/coroutine
288

Can __builtin_coro_noop produce a constant expression?

GorNishanov marked 2 inline comments as done.Apr 4 2018, 7:00 AM
GorNishanov added inline comments.
include/experimental/coroutine
288

No. llvm generates this value, so from clang perspective, it is not a constant.
At llvm level it is a private per TU constant, so invocations of noop_coroutine() in different TUs linked into the same program will return you different values.

294

Those were added by @EricWF, so from my perspective they are immutable.

CaseyCarter added inline comments.
include/experimental/coroutine
268

This file can't seem to decide if it's using two-space indents or four-space indents. It would be nice to pick one and make the whole thing consistent.

274

If __builtin_coro_promise returns a void*, this reinterpret_cast should be a static_cast. (and on 216.)

275

Is this->non_static_member_of_non_dependent_base_class idiomatic in libc++? I typically reserve this-> for forcing lookup into dependent bases. (and on 217.)

278

Should these be _LIBCPP_CONSTEXPR?

286

There's an extra space here between () and _NOEXCEPT.

288

If this class type is not literal - since there's no constexpr constructor - applying constexpr to the member functions on 278-283 is at best misleading and at worst ill-formed NDR due to http://eel.is/c++draft/dcl.constexpr#5.

288

This constructor should be _NOEXCEPT, since it's invoked by noop_coroutine which is _NOEXCEPT.

test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
21

My inner @STL_MSFT wants these includes to be sorted lexicographically ;)

23

coroutines_v1?

56

Should we assert(h && !h.done()); after each of these calls on 55-57 to validate that they have no effect?

GorNishanov marked 2 inline comments as done.
  • static_cast instead of reintrepret_cast in promise()
  • 2 spaces indent in added code (the rest of the file stayed as is)
  • added static_assert to check for done-ness and capacity
  • constexpr => _LIBCPP_CONSTEXPR
  • noexcept => _NOEXCEPT
GorNishanov marked 11 inline comments as done.Apr 4 2018, 10:04 AM
GorNishanov added inline comments.
include/experimental/coroutine
275

I am keeping it consistent with other uses in the file.

288

Issue for Rapperswil? These constexprs were approved by LEWG/LWG in Jacksonville 2018

EricWF added inline comments.Apr 4 2018, 10:34 AM
include/experimental/coroutine
288

@CaseyCarter: I'm not sure this is true. Clang seems to be able to evaluate constexpr member functions of a non-literal type so long as they don't depend on the "argument value" of this. Example:

struct T {
  T() {}
  constexpr bool foo() const { return true; }
};
T t;
static_assert(t.foo(), "");
294

I'm not sure what I was thinking when I implemented these things with _LIBCPP_ALWAYS_INLINE.

CaseyCarter accepted this revision.Apr 4 2018, 10:45 AM
CaseyCarter added inline comments.
include/experimental/coroutine
288

@EricWF Yes, thank you - I'm forgetting that an invocation of a constexpr non-static member function with a non-constant-expression implicit object parameter can appear in a constant expression if it doesn't perform lvalue-to-rvalue conversion on the implicit object parameter.

These constexprs aren't ill-formed NDR, but they do seem pointless. The base class versions aren't constexpr, so I can only use these when I know the concrete type of my handle is noop_coroutine_handle, in which case I know the results and have no need to call them at run time *or* compile time.

*shrug* I suppose they do no harm.

GorNishanov marked an inline comment as done.Apr 4 2018, 11:14 AM
GorNishanov added inline comments.
include/experimental/coroutine
294

@EricWF would you like me to do wholesale search-and-replace in coroutine header and make everything _LIBCPP_INLINE_VISIBILITY. Though, strategic use of always_inline in coroutine_handle and related classes can allow heap allocation elision to work even in -O0

EricWF added inline comments.Apr 4 2018, 11:16 AM
include/experimental/coroutine
294

_LIBCPP_INLINE_VISIBILITY implies always inline most of the time. I wouldn't mind you replacing all instances in the header.

  • s/_LIBCPP_ALWAYS_INLINE/_LIBCPP_INLINE_VISIBILITY throughout <experimental/coroutine>
  • Added _LIBCPP_INLINE_VISIBILITY to noop_coroutine constructor

@EricWF , good to go?

GorNishanov marked an inline comment as done.Apr 4 2018, 11:41 AM
EricWF accepted this revision.Apr 4 2018, 11:43 AM

Yeah, LGTM.