[libcxx-commits] [PATCH] D116144: [libcxx] [Coroutines] Support noop_coroutine for GCC

[Chuanqi Xu via Phabricator via libcxx-commits]

ChuanqiXu added a comment.

I added @iains  who is the original author of libstdc++'s coroutine. I am not sure if Iain would take a look. But I believe the implementation might be correct since I looked at the implementation of libstdc++'s implementation.



================
Comment at: libcxx/include/__coroutine/noop_coroutine_handle.h:79
+    // frame for noop_coroutine from source instead.
+    struct __noop_coroutine__frame_ty_ {
+        static void __dummy_resume_destroy_func_() { }
----------------
Quuxplusone wrote:
> The double-underscore here seems suspicious. Could you make it a single underscore? or is this //specific identifier// magic to GCC somehow?
I guess you are referring the double-underscore in the middle instead of the start, right? My bad. It is my typo.


================
Comment at: libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:24
 
-#if __has_builtin(__builtin_coro_noop)
+#if __has_builtin(__builtin_coro_noop) || defined(_LIBCPP_COMPILER_GCC)
 
----------------
Quuxplusone wrote:
> At this point, I think you could just remove this `#if` and start using the usual annotations, e.g. `// UNSUPPORTED: clang-13` or whatever.
I think the line `// UNSUPPORTED: libcpp-no-coroutines` would filter `clang-13` out.


================
Comment at: libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:71-72
+  base();
+  assert(base);
+  assert(base.done() == false);
+
----------------
Quuxplusone wrote:
> Let's keep these consistent with lines 60–61. I have no preference on the order or whether they end with `, ""`, but let's be consistent one way or the other.
We can't do that. Since `noop_coroutine_handle::done()` is constexpr and `std::coroutine_handle<>::done()` isn't constexpr.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116144/new/

https://reviews.llvm.org/D116144