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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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_() { }
----------------
ChuanqiXu wrote:
> 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.
Yep, I meant the double-underscore in the middle.

Also, please remove the trailing underscore in `__dummy_resume_destroy_func_`. Nobody's 100% sure whether we use trailing-underscore for //data// members or //private// members ;) but `__dummy_resume_destroy_func_` is neither private nor data, so it definitely shouldn't have one. Just make it `__dummy_resume_destroy_func()`. 



================
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)
 
----------------
ChuanqiXu wrote:
> 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.
No problem. I was just trying to guess the name of some compiler that might fail the condition `#if __has_builtin(__builtin_coro_noop) || defined(_LIBCPP_COMPILER_GCC)`.


================
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);
+
----------------
ChuanqiXu wrote:
> 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.
Oops, I hadn't noticed that these were regular `assert` instead of `static_assert`. So they can't use `, ""`; but at least you can put them in the same order: i.e., please swap lines 69 and 70.


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

https://reviews.llvm.org/D116144