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

Casey Carter via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 4 08:15:55 PDT 2018


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_promise> 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 <type_traits>
+#include <cassert>
+
----------------
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





More information about the cfe-commits mailing list