[coroutines] Add noop_coroutine to <experimental/coroutine>

[GorNishanov]

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

[GorNishanov]

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

[lewissbaker]

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

[EricWF]

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

[GorNishanov]

In D45121#1056408, @lewissbaker wrote:

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

[GorNishanov]

incorporated review feedback

[lewissbaker]

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]

include/experimental/coroutine

288

Can __builtin_coro_noop produce a constant expression?

[GorNishanov]

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]

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]

• 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]

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]

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]

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]

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]

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.

[GorNishanov]

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

@EricWF , good to go?

[EricWF]

Yeah, LGTM.

[GorNishanov]

Committed as:

https://reviews.llvm.org/rCXX329237

and a flurry of cleanup fixes:

https://reviews.llvm.org/rCXX329239
https://reviews.llvm.org/rCXX329240
https://reviews.llvm.org/rCXX329245

include/experimental/coroutine

static coroutine_handle from_promise(_Promise& __promise) _NOEXCEPT {
         coroutine_handle __tmp;
         __tmp.__handle_ = __builtin_coro_promise(
             _VSTD::addressof(const_cast<_RawPromise&>(__promise)),
              __alignof(_Promise), true);
         return __tmp;
     }
 };
 
+struct noop_coroutine_promise {};
+
+template <>
+class _LIBCPP_TEMPLATE_VIS coroutine_handle<noop_coroutine_promise>
+    : public coroutine_handle<> {
+  using _Base = coroutine_handle<>;
+  using _Promise = noop_coroutine_promise;
+public:
+
+    _LIBCPP_INLINE_VISIBILITY
+    _Promise& promise() const {
+        return *reinterpret_cast<_Promise*>(
+            __builtin_coro_promise(this->__handle_, __alignof(_Promise), false));
+    }
+
+    constexpr explicit operator bool() const noexcept { return true; }
+    constexpr bool done() const noexcept { return false; }
+
+    constexpr void operator()() const noexcept {}
+    constexpr void resume() const noexcept {}
+    constexpr void destroy() const noexcept {}
+
+private:
+    friend coroutine_handle<noop_coroutine_promise> noop_coroutine()  _NOEXCEPT;
+
+    coroutine_handle() {
+      this->__handle_ = __builtin_coro_noop();
+    }
+};
+
+using noop_coroutine_handle = coroutine_handle<noop_coroutine_promise>;
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {
+  return {};
+}
+
 struct _LIBCPP_TYPE_VIS suspend_never {
   _LIBCPP_ALWAYS_INLINE
   bool await_ready() const _NOEXCEPT { return true; }
   _LIBCPP_ALWAYS_INLINE
   void await_suspend(coroutine_handle<>) const _NOEXCEPT {}
   _LIBCPP_ALWAYS_INLINE
   void await_resume() const _NOEXCEPT {}
 };

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

+// -*- 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
+
+// <experimental/coroutine>
+// struct noop_coroutine_promise;
+// using noop_coroutine_handle = coroutine_handle<noop_coroutine_promise>;
+// noop_coroutine_handle noop_coroutine() noexcept;
+
+#include <experimental/coroutine>
+#include <type_traits>
+#include <cassert>
+
+namespace coro = std::experimental;
+
+static_assert(std::is_same<coro::coroutine_handle<coro::noop_coroutine_promise>, coro::noop_coroutine_handle>::value, "");
+static_assert(std::is_same<decltype(coro::noop_coroutine()), coro::noop_coroutine_handle>::value, "");
+
+// template <> struct coroutine_handle<noop_coroutine_promise> : coroutine_handle<>
+// {
+// // 18.11.2.7 noop observers
+// constexpr explicit operator bool() const noexcept;
+// constexpr bool done() const noexcept;
+
+// // 18.11.2.8 noop resumption
+// constexpr void operator()() const noexcept;
+// constexpr void resume() const noexcept;
+// constexpr void destroy() const noexcept;
+
+// // 18.11.2.9 noop promise access
+// noop_coroutine_promise& promise() const noexcept;
+
+// // 18.11.2.10 noop address
+// constexpr void* address() const noexcept;
+
+int main()
+{
+  auto h = coro::noop_coroutine();
+  coro::coroutine_handle<> base = h;
+
+  assert(h);
+  assert(base);
+
+  assert(!h.done());
+  assert(!base.done());
+
+  h.resume();
+  h.destroy();
+  h();
+
+  h.promise();
+  assert(h.address() == base.address());
+  assert(h.address() != nullptr);
+}
+