[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

63

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

13

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

 };
 
 template <typename _Promise = void>
 class _LIBCPP_TEMPLATE_VIS coroutine_handle;
 
 template <>
 class _LIBCPP_TEMPLATE_VIS coroutine_handle<void> {
 public:
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR coroutine_handle() _NOEXCEPT : __handle_(nullptr) {}
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR coroutine_handle(nullptr_t) _NOEXCEPT : __handle_(nullptr) {}
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     coroutine_handle& operator=(nullptr_t) _NOEXCEPT {
         __handle_ = nullptr;
         return *this;
     }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR void* address() const _NOEXCEPT { return __handle_; }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     _LIBCPP_CONSTEXPR explicit operator bool() const _NOEXCEPT { return __handle_; }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     void operator()() { resume(); }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     void resume() {
       _LIBCPP_ASSERT(__is_suspended(),
                      "resume() can only be called on suspended coroutines");
       _LIBCPP_ASSERT(!done(),
                 "resume() has undefined behavior when the coroutine is done");
       __builtin_coro_resume(__handle_);
     }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     void destroy() {
       _LIBCPP_ASSERT(__is_suspended(),
                      "destroy() can only be called on suspended coroutines");
       __builtin_coro_destroy(__handle_);
     }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     bool done() const {
       _LIBCPP_ASSERT(__is_suspended(),
                      "done() can only be called on suspended coroutines");
       return __builtin_coro_done(__handle_);
     }
 
 public:
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     static coroutine_handle from_address(void* __addr) _NOEXCEPT {
         coroutine_handle __tmp;
         __tmp.__handle_ = __addr;
         return __tmp;
     }
 
     // FIXME: Should from_address(nullptr) be allowed?
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     static coroutine_handle from_address(nullptr_t) _NOEXCEPT {
       return coroutine_handle(nullptr);
     }
 
     template <class _Tp, bool _CallIsValid = false>
     static coroutine_handle from_address(_Tp*) {
       static_assert(_CallIsValid,
        "coroutine_handle<void>::from_address cannot be called with "
         "non-void pointers");
     }
 
 private:
   bool __is_suspended() const _NOEXCEPT  {
     // FIXME actually implement a check for if the coro is suspended.
     return __handle_;
   }
 
   template <class _PromiseT> friend class coroutine_handle;
   void* __handle_;
 };
 
 // 18.11.2.7 comparison operators:
-inline _LIBCPP_ALWAYS_INLINE
+inline _LIBCPP_INLINE_VISIBILITY
 bool operator==(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
     return __x.address() == __y.address();
 }
-inline _LIBCPP_ALWAYS_INLINE
+inline _LIBCPP_INLINE_VISIBILITY
 bool operator!=(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
     return !(__x == __y);
 }
-inline _LIBCPP_ALWAYS_INLINE
+inline _LIBCPP_INLINE_VISIBILITY
 bool operator<(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
     return less<void*>()(__x.address(), __y.address());
 }
-inline _LIBCPP_ALWAYS_INLINE
+inline _LIBCPP_INLINE_VISIBILITY
 bool operator>(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
     return __y < __x;
 }
-inline _LIBCPP_ALWAYS_INLINE
+inline _LIBCPP_INLINE_VISIBILITY
 bool operator<=(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
     return !(__x > __y);
 }
-inline _LIBCPP_ALWAYS_INLINE
+inline _LIBCPP_INLINE_VISIBILITY
 bool operator>=(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
     return !(__x < __y);
 }
 
 template <typename _Promise>
 class _LIBCPP_TEMPLATE_VIS coroutine_handle : public coroutine_handle<> {
     using _Base = coroutine_handle<>;
 public:
 #ifndef _LIBCPP_CXX03_LANG
     // 18.11.2.1 construct/reset
     using coroutine_handle<>::coroutine_handle;
 #else
-    _LIBCPP_ALWAYS_INLINE coroutine_handle() _NOEXCEPT : _Base() {}
-    _LIBCPP_ALWAYS_INLINE coroutine_handle(nullptr_t) _NOEXCEPT : _Base(nullptr) {}
+    _LIBCPP_INLINE_VISIBILITY coroutine_handle() _NOEXCEPT : _Base() {}
+    _LIBCPP_INLINE_VISIBILITY coroutine_handle(nullptr_t) _NOEXCEPT : _Base(nullptr) {}
 #endif
     _LIBCPP_INLINE_VISIBILITY
     coroutine_handle& operator=(nullptr_t) _NOEXCEPT {
         _Base::operator=(nullptr);
         return *this;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     _Promise& promise() const {
-        return *reinterpret_cast<_Promise*>(
+        return *static_cast<_Promise*>(
             __builtin_coro_promise(this->__handle_, __alignof(_Promise), false));
     }
 
 public:
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     static coroutine_handle from_address(void* __addr) _NOEXCEPT {
         coroutine_handle __tmp;
         __tmp.__handle_ = __addr;
         return __tmp;
     }
 
     // NOTE: this overload isn't required by the standard but is needed so
     // the deleted _Promise* overload doesn't make from_address(nullptr)
     // ambiguous.
     // FIXME: should from_address work with nullptr?
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     static coroutine_handle from_address(nullptr_t) _NOEXCEPT {
       return coroutine_handle(nullptr);
     }
 
     template <class _Tp, bool _CallIsValid = false>
     static coroutine_handle from_address(_Tp*) {
       static_assert(_CallIsValid,
        "coroutine_handle<promise_type>::from_address cannot be called with "
         "non-void pointers");
     }
 
     template <bool _CallIsValid = false>
     static coroutine_handle from_address(_Promise*) {
       static_assert(_CallIsValid,
        "coroutine_handle<promise_type>::from_address cannot be used with "
         "pointers to the coroutine's promise type; use 'from_promise' instead");
     }
 
-    _LIBCPP_ALWAYS_INLINE
+    _LIBCPP_INLINE_VISIBILITY
     static coroutine_handle from_promise(_Promise& __promise) _NOEXCEPT {
         typedef typename remove_cv<_Promise>::type _RawPromise;
         coroutine_handle __tmp;
         __tmp.__handle_ = __builtin_coro_promise(
             _VSTD::addressof(const_cast<_RawPromise&>(__promise)),
              __alignof(_Promise), true);
         return __tmp;
     }
 };
 
+#if __has_builtin(__builtin_coro_noop)
+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 *static_cast<_Promise*>(
+      __builtin_coro_promise(this->__handle_, __alignof(_Promise), false));
+  }
+
+  _LIBCPP_CONSTEXPR explicit operator bool() const _NOEXCEPT { return true; }
+  _LIBCPP_CONSTEXPR bool done() const _NOEXCEPT { return false; }
+
+  _LIBCPP_CONSTEXPR void operator()() const _NOEXCEPT {}
+  _LIBCPP_CONSTEXPR void resume() const _NOEXCEPT {}
+  _LIBCPP_CONSTEXPR void destroy() const _NOEXCEPT {}
+
+private:
+  friend coroutine_handle<noop_coroutine_promise> noop_coroutine() _NOEXCEPT;
+
+  _LIBCPP_INLINE_VISIBILITY coroutine_handle() _NOEXCEPT {
+    this->__handle_ = __builtin_coro_noop();
+  }
+};
+
+using noop_coroutine_handle = coroutine_handle<noop_coroutine_promise>;
+
+inline _LIBCPP_INLINE_VISIBILITY
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {
+  return {};
+}
+#endif // __has_builtin(__builtin_coro_noop)
+
 struct _LIBCPP_TYPE_VIS suspend_never {
-  _LIBCPP_ALWAYS_INLINE
+  _LIBCPP_INLINE_VISIBILITY
   bool await_ready() const _NOEXCEPT { return true; }
-  _LIBCPP_ALWAYS_INLINE
+  _LIBCPP_INLINE_VISIBILITY
   void await_suspend(coroutine_handle<>) const _NOEXCEPT {}
-  _LIBCPP_ALWAYS_INLINE
+  _LIBCPP_INLINE_VISIBILITY
   void await_resume() const _NOEXCEPT {}
 };
 
 struct _LIBCPP_TYPE_VIS suspend_always {
-  _LIBCPP_ALWAYS_INLINE
+  _LIBCPP_INLINE_VISIBILITY
   bool await_ready() const _NOEXCEPT { return false; }
-  _LIBCPP_ALWAYS_INLINE
+  _LIBCPP_INLINE_VISIBILITY
   void await_suspend(coroutine_handle<>) const _NOEXCEPT {}
-  _LIBCPP_ALWAYS_INLINE
+  _LIBCPP_INLINE_VISIBILITY
   void await_resume() const _NOEXCEPT {}
 };
 
 _LIBCPP_END_NAMESPACE_EXPERIMENTAL_COROUTINES
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>

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
+// XFAIL: clang-5, clang-6
+// <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 <cassert>
+#include <type_traits>
+
+namespace coro = std::experimental::coroutines_v1;
+
+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();
+  static_assert(h.done() == false, "");
+  static_assert(h, "");
+
+  h.promise();
+  assert(h.address() == base.address());
+  assert(h.address() != nullptr);
+  assert(coro::coroutine_handle<>::from_address(h.address()) == base);
+}
+