[PATCH] D45121: [coroutines] Add noop_coroutine to <experimental/coroutine>

[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