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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
