[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov closed this revision.
GorNishanov added a comment.

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


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Eric Fiselier via Phabricator via cfe-commits]

EricWF accepted this revision.
EricWF added a comment.

Yeah, LGTM.


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 141010.
GorNishanov added a comment.

- s/_LIBCPP_ALWAYS_INLINE/_LIBCPP_INLINE_VISIBILITY throughout 

- Added _LIBCPP_INLINE_VISIBILITY to noop_coroutine constructor

@EricWF , good to go?


https://reviews.llvm.org/D45121

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

Index: test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
===
--- /dev/null
+++ test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
@@ -0,0 +1,66 @@
+// -*- 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
+// 
+// struct noop_coroutine_promise;
+// using noop_coroutine_handle = coroutine_handle;
+// noop_coroutine_handle noop_coroutine() noexcept;
+
+#include 
+#include 
+#include 
+
+namespace coro = std::experimental::coroutines_v1;
+
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+
+// template <> struct coroutine_handle : 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);
+}
+
Index: include/experimental/coroutine
===
--- include/experimental/coroutine
+++ include/experimental/coroutine
@@ -93,60 +93,60 @@
 template <>
 class _LIBCPP_TEMPLATE_VIS coroutine_handle {
 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 

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

GorNishanov wrote:
> EricWF wrote:
> > GorNishanov wrote:
> > > lewissbaker wrote:
> > > > EricWF wrote:
> > > > > This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use 
> > > > > `_LIBCPP_ALWAYS_INLINE` in new code.
> > > > 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?
> > > Those were added by @EricWF, so from my perspective they are immutable.
> > I'm not sure what I was thinking when I implemented these things with 
> > `_LIBCPP_ALWAYS_INLINE`.
> @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
`_LIBCPP_INLINE_VISIBILITY` implies always inline most of the time. I wouldn't 
mind you replacing all instances in the header.


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov marked an inline comment as done.
GorNishanov added inline comments.



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

EricWF wrote:
> GorNishanov wrote:
> > lewissbaker wrote:
> > > EricWF wrote:
> > > > This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use 
> > > > `_LIBCPP_ALWAYS_INLINE` in new code.
> > > 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?
> > Those were added by @EricWF, so from my perspective they are immutable.
> I'm not sure what I was thinking when I implemented these things with 
> `_LIBCPP_ALWAYS_INLINE`.
@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


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter accepted this revision.
CaseyCarter added inline comments.



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

EricWF wrote:
> GorNishanov wrote:
> > CaseyCarter wrote:
> > > CaseyCarter wrote:
> > > > GorNishanov wrote:
> > > > > EricWF wrote:
> > > > > > Can `__builtin_coro_noop` produce a constant expression?
> > > > > 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.
> > > > 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.
> > > This constructor should be `_NOEXCEPT`, since it's invoked by 
> > > `noop_coroutine` which is `_NOEXCEPT`.
> > Issue for Rapperswil? These constexprs were approved by LEWG/LWG in 
> > Jacksonville 2018
> @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(), "");
> ```
@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 `constexpr`s 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.


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

GorNishanov wrote:
> lewissbaker wrote:
> > EricWF wrote:
> > > This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use 
> > > `_LIBCPP_ALWAYS_INLINE` in new code.
> > 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?
> Those were added by @EricWF, so from my perspective they are immutable.
I'm not sure what I was thinking when I implemented these things with 
`_LIBCPP_ALWAYS_INLINE`.



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

GorNishanov wrote:
> CaseyCarter wrote:
> > CaseyCarter wrote:
> > > GorNishanov wrote:
> > > > EricWF wrote:
> > > > > Can `__builtin_coro_noop` produce a constant expression?
> > > > 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.
> > > 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.
> > This constructor should be `_NOEXCEPT`, since it's invoked by 
> > `noop_coroutine` which is `_NOEXCEPT`.
> Issue for Rapperswil? These constexprs were approved by LEWG/LWG in 
> Jacksonville 2018
@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(), "");
```


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov marked 11 inline comments as done.
GorNishanov added inline comments.



Comment at: include/experimental/coroutine:275
+return *reinterpret_cast<_Promise*>(
+__builtin_coro_promise(this->__handle_, __alignof(_Promise), 
false));
+}

CaseyCarter wrote:
> 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.)
I am keeping it consistent with other uses in the file. 



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

CaseyCarter wrote:
> CaseyCarter wrote:
> > GorNishanov wrote:
> > > EricWF wrote:
> > > > Can `__builtin_coro_noop` produce a constant expression?
> > > 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.
> > 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.
> This constructor should be `_NOEXCEPT`, since it's invoked by 
> `noop_coroutine` which is `_NOEXCEPT`.
Issue for Rapperswil? These constexprs were approved by LEWG/LWG in 
Jacksonville 2018


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 140983.
GorNishanov marked 2 inline comments as done.
GorNishanov added a comment.

- 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


https://reviews.llvm.org/D45121

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

Index: test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
===
--- /dev/null
+++ test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
@@ -0,0 +1,66 @@
+// -*- 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
+// 
+// struct noop_coroutine_promise;
+// using noop_coroutine_handle = coroutine_handle;
+// noop_coroutine_handle noop_coroutine() noexcept;
+
+#include 
+#include 
+#include 
+
+namespace coro = std::experimental::coroutines_v1;
+
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+
+// template <> struct coroutine_handle : 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);
+}
+
Index: include/experimental/coroutine
===
--- include/experimental/coroutine
+++ include/experimental/coroutine
@@ -213,7 +213,7 @@
 
 _LIBCPP_INLINE_VISIBILITY
 _Promise& promise() const {
-return *reinterpret_cast<_Promise*>(
+return *static_cast<_Promise*>(
 __builtin_coro_promise(this->__handle_, __alignof(_Promise), false));
 }
 
@@ -259,6 +259,45 @@
 }
 };
 
+#if __has_builtin(__builtin_coro_noop)
+struct noop_coroutine_promise {};
+
+template <>
+class _LIBCPP_TEMPLATE_VIS coroutine_handle
+: 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() _NOEXCEPT;
+
+  coroutine_handle() _NOEXCEPT {
+this->__handle_ = __builtin_coro_noop();
+  }
+};
+
+using noop_coroutine_handle = coroutine_handle;
+
+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
   bool await_ready() const _NOEXCEPT { return true; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter added a subscriber: STL_MSFT.
CaseyCarter added inline comments.



Comment at: include/experimental/coroutine:268
+: public coroutine_handle<> {
+  using _Base = coroutine_handle<>;
+  using _Promise = noop_coroutine_promise;

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.



Comment at: include/experimental/coroutine:274
+_Promise& promise() const {
+return *reinterpret_cast<_Promise*>(
+__builtin_coro_promise(this->__handle_, __alignof(_Promise), 
false));

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



Comment at: include/experimental/coroutine:275
+return *reinterpret_cast<_Promise*>(
+__builtin_coro_promise(this->__handle_, __alignof(_Promise), 
false));
+}

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



Comment at: include/experimental/coroutine:278
+
+constexpr explicit operator bool() const noexcept { return true; }
+constexpr bool done() const noexcept { return false; }

Should these be `_LIBCPP_CONSTEXPR`?



Comment at: include/experimental/coroutine:286
+private:
+friend coroutine_handle noop_coroutine()  
_NOEXCEPT;
+

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



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

GorNishanov wrote:
> EricWF wrote:
> > Can `__builtin_coro_noop` produce a constant expression?
> 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.
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.



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

CaseyCarter wrote:
> GorNishanov wrote:
> > EricWF wrote:
> > > Can `__builtin_coro_noop` produce a constant expression?
> > 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.
> 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.
This constructor should be `_NOEXCEPT`, since it's invoked by `noop_coroutine` 
which is `_NOEXCEPT`.



Comment at: 
test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:20
+#include 
+#include 
+

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



Comment at: 
test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:22
+
+namespace coro = std::experimental;
+

`coroutines_v1`?



Comment at: 
test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:55
+
+  h.resume();
+  h.destroy();

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


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov marked 2 inline comments as done.
GorNishanov added inline comments.



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

lewissbaker wrote:
> EricWF wrote:
> > This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use 
> > `_LIBCPP_ALWAYS_INLINE` in new code.
> 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?
Those were added by @EricWF, so from my perspective they are immutable.



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

EricWF wrote:
> Can `__builtin_coro_noop` produce a constant expression?
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.


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/experimental/coroutine:288
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();

Can `__builtin_coro_noop` produce a constant expression?


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

EricWF wrote:
> This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use 
> `_LIBCPP_ALWAYS_INLINE` in new code.
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?


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 140903.
GorNishanov added a comment.

incorporated review feedback


https://reviews.llvm.org/D45121

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

Index: test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
===
--- /dev/null
+++ test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
@@ -0,0 +1,64 @@
+// -*- 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
+// 
+// struct noop_coroutine_promise;
+// using noop_coroutine_handle = coroutine_handle;
+// noop_coroutine_handle noop_coroutine() noexcept;
+
+#include 
+#include 
+#include 
+
+namespace coro = std::experimental;
+
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+
+// template <> struct coroutine_handle : 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);
+  assert(coro::coroutine_handle<>::from_address(h.address()) == base);
+}
+
Index: include/experimental/coroutine
===
--- include/experimental/coroutine
+++ include/experimental/coroutine
@@ -259,6 +259,45 @@
 }
 };
 
+#if __has_builtin(__builtin_coro_noop)
+struct noop_coroutine_promise {};
+
+template <>
+class _LIBCPP_TEMPLATE_VIS coroutine_handle
+: 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()  _NOEXCEPT;
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();
+}
+};
+
+using noop_coroutine_handle = coroutine_handle;
+
+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
   bool await_ready() const _NOEXCEPT { return true; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov added a comment.

In https://reviews.llvm.org/D45121#1056408, @lewissbaker wrote:

> The `coroutine_handle` type does not have a 
> `from_address()` or a `from_promise()` static functions in the same way that 
> the `coroutine_handle` 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` 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()`


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/experimental/coroutine:262
 
+struct noop_coroutine_promise {};
+

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



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

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



Comment at: 
test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:12
+// UNSUPPORTED: c++98, c++03, c++11
+
+// 

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


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Lewis Baker via Phabricator via cfe-commits]

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

The `coroutine_handle` type does not have a 
`from_address()` or a `from_promise()` static functions in the same way that 
the `coroutine_handle` 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` then you 
can just use `noop_coroutine()` to get the handle instead.




Comment at: 
test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:62
+  assert(h.address() != nullptr);
+}
+

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


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov added a comment.

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


https://reviews.llvm.org/D45121



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

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov created this revision.
GorNishanov added reviewers: EricWF, lewissbaker, modocache.
GorNishanov added dependencies: D45114: [coroutines] Add support for 
llvm.coro.noop intrinsics, D45120: [coroutines] Add __builtin_coro_noop => 
llvm.coro.noop.

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 

Related clang and llvm patches:

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


https://reviews.llvm.org/D45121

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

Index: test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
===
--- /dev/null
+++ test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp
@@ -0,0 +1,63 @@
+// -*- 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
+
+// 
+// struct noop_coroutine_promise;
+// using noop_coroutine_handle = coroutine_handle;
+// noop_coroutine_handle noop_coroutine() noexcept;
+
+#include 
+#include 
+#include 
+
+namespace coro = std::experimental;
+
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+
+// template <> struct coroutine_handle : 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);
+}
+
Index: include/experimental/coroutine
===
--- include/experimental/coroutine
+++ include/experimental/coroutine
@@ -259,6 +259,43 @@
 }
 };
 
+struct noop_coroutine_promise {};
+
+template <>
+class _LIBCPP_TEMPLATE_VIS coroutine_handle
+: 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()  _NOEXCEPT;
+
+coroutine_handle() {
+  this->__handle_ = __builtin_coro_noop();
+}
+};
+
+using noop_coroutine_handle = coroutine_handle;
+
+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; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits