This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [coroutine] Conform coroutine implementation
ClosedPublic

Authored by ChuanqiXu on Sep 8 2021, 4:40 AM.

Details

Summary

This revision is based on @Quuxplusone's suggestion in D108697.
This patch aims to conform the coroutine implementation to C++20 standard.

The reference for this patch is: https://eel.is/c++draft/support.coroutine

The main change of this patch includes:

  • Require _LIBCPP_STD_VER > 11. This keeps consistency with tests and gcc.
  • Replace _LIBCPP_CONSTEXPR and _NOEXCEPT with constexpr and noexcept.
  • Add missing const and noexcept qualifier for functions.
  • Break the inheritance relationship of coroutine_handle<Promise>, coroutine_handle<void> and noop_coroutine_handle based on the specification.
  • Remove redundant overloads for from_address.
  • Use operator<=> to replace <, >, =>, <=.
  • Use stable names throughout like` [coroutine.handle.compare] to replace something like 18.11.2.7 comparison operators`.
  • Use _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY.

Test Plan: check-all

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This comment was removed by ChuanqiXu.

Remove the issue about [coroutine.handle.compare] in SpaceshipProjects.csv.

ldionne added inline comments.Sep 23 2021, 7:47 AM
libcxx/docs/Status/SpaceshipProjects.csv
12

We don't remove issues like that, we mark them as complete. Please take a look at the surrounding code.

ChuanqiXu updated this revision to Diff 374712.Sep 23 2021, 7:28 PM

Mention <coroutine> in libcxx/utils/generate_header_tests.py according to the opinions of @ldionne in D108697.

I haven't fully reviewed the tests. I know they're just a copy of the existing experimental tests; but hey, this is also our best chance to audit them and get them right for C++20!
Have we got any Coroutines experts in the house who might review the tests for sanity and/or blind spots? (@lewissbaker?)

I think it might be appreciated to break down the monolithic <coroutine> header into sub-headers, i.e.

  • __coroutine/coroutine_handle.h
  • __coroutine/coroutine_traits.h
  • __coroutine/noop_coroutine_handle.h
  • __coroutine/suspend_always.h
  • __coroutine/suspend_never.h

leaving the <coroutine> header itself as just a synopsis comment and a bunch of #includes. (See the recent precedent in <compare>, for example.) I'm not asking you to do that, necessarily, because I'm not 100% sure it wouldn't be wasted work; but if you did do it, I'd estimate better than 50% chance it would be appreciated. :)

libcxx/include/CMakeLists.txt
331

You also need to add it to libcxx/include/module.modulemap (by copying one of the existing headers' entries, with appropriate modifications).

libcxx/include/coroutine
18

http://eel.is/c++draft/coroutine.syn doesn't say anything about coroutines_v1.

  • Do we need this inline namespace for ABI reasons? (No?)
  • If we need it, do we need it in the synopsis comment? (No.)
  • If we need it, should we hide it behind a macro? (Yes.)

The precedent on all these questions is <filesystem> and its macro _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM.

21–25

Style nit: I'd love to see template <class ~~~> here and throughout the actual code, both to match the Standard's style and to shorten the lines. :)

46–52

Alphabetize (__config, __debug, __functional/hash.h, compare, ...).

<strikethru>Can we get away with just <__compare/ordering.h> instead of all of <compare>?</strikethru> No, because the inclusion is mandated by [coroutine.syn]. OK.

58–64

I think lines 57-64 should just be deleted. We already guard the whole header under _LIBCPP_HAS_NO_COROUTINES (line 65), so there's not really any point in making it fail noisily here, right?

105
_LIBCPP_HIDE_FROM_ABI
coroutine_handle() = default;

Defaulting it will give us constexpr and noexcept for free.

244

IMHO return __handle_ != nullptr; for clarity would be nice. No biggie.
Ditto line 277 below.

272

Line 214 above implies that _Promise might be a cv-qualified type; like, you could have a coroutine_handle<const Widget>. Do we need to ask LWG if that's intentional?

  • If so, then static_cast here might not be good enough, right?
  • If not, then line 214 could be simplified, right?
  • Do we have any tests for coroutine_handle<const Widget>?
362

s/> >/>>/
And, on line 362, you don't really need the __arg_type typedef, do you?

libcxx/test/std/language.support/support.coroutines/coroutine.traits/promise_type.pass.cpp
36–49

Never reopen namespace std in user code.

template<>
struct std::coroutine_traits<A, int> { using promise_type = int*; };
template<class... Args>
struct std::coroutine_traits<B, Args...> { using promise_type = B*; };
template<>
struct std::coroutine_traits<C> { using promise_type = void; };
libcxx/test/std/language.support/support.coroutines/coroutine.trivial.awaitables/suspend_always.pass.cpp
19

Personally I think I'd like to see this type alias inlined everywhere it's used. I don't think it's buying us anything to have an alias.

26–27

The (void) uses aren't needed, because these variables are used, on the next two lines.

44–45

Each of these typedefs is used only once; I don't think they're buying you anything either.
(And ditto throughout the new tests.)
I'm happy with std::coroutine_handle<> h; — we don't need long variable names — but it seems silly to spend extra lines just to introduce single-character typedefs that we'll never use again.

50

Nit: ASSERT_NOEXCEPT(s.await_ready());
(and similarly throughout all the new tests)

80–81

IIUC, the point is just to test that you can make a suspend_always object at constexpr time? You're already successfully testing that, on lines 73 and 76, because that constexpr function creates a local variable. So you can remove constexpr_sa.
You can also remove safe_sa. IIUC, TEST_SAFE_STATIC is basically a pre-'20 constinit, and that's just testing that you can make a constinit variable of type suspend_always. But if you can make a constexpr variable (which you can, then you can certainly make a constinit one! So, remove safe_sa too.

libcxx/test/std/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
32–38

I think it would be perfectly sufficient to mark this test UNSUPPORTED: c++03, c++11, c++14. And write a second test file specifically for REQUIRES: c++14 if you think it's very important to test this codepath. People using std coroutines in C++14 mode deserve what they get, I think. :P

102

If (void) is needed here, something is gravely wrong with C++20. :P
Ditto line 74 above.

libcxx/test/std/language.support/support.coroutines/end.to.end/generator.pass.cpp
2

Please remove any // -*- C++ -*- lines from files with the .cpp extension. I'm surely going to land D110874 before you land this.

23

Let's eliminate this using namespace std. It's easier to git grep std::suspend_always when it's not hidden under an unqualified spelling.

libcxx/test/std/language.support/support.coroutines/end.to.end/oneshot_func.pass.cpp
79–80

IIUC, after C++17 guaranteed copy elision, this test is exactly equivalent to

func<int> f = func<int>::Create([]() { yielded_values.push_back(43); return 44; });
yielded_values.push_back(f());
assert((yielded_values == std::vector<int>{43, 44}));

which is basically the same as

func<int> f = func<int>::Create([]() { return 44; });
assert(f() == 44);

(because obviously if f() comes out to 44 in particular, then the body of the lambda must have run; and so we don't need to put any other side-effect inside the lambda to "prove" that it ran).

libcxx/test/std/language.support/support.coroutines/includes.pass.cpp
14 ↗(On Diff #374712)

http://eel.is/c++draft/coroutine.syn doesn't say anything about having to include <new>.
I'd say delete this test... and then also delete #include <new> from <coroutine>, unless it's actually needed (which it might be).

libcxx/test/std/language.support/support.coroutines/lit.local.cfg
1–7 ↗(On Diff #374712)

I vaguely remember we talked about this somewhere; but now I'm skeptical of it. If you simply remove this lit.local.cfg file, what breaks?

libcxx/test/support/coroutine_types.h
0

"coroutine_types.h" is used in literally one test: std/experimental/language.support/support.coroutines/end.to.end/generator.pass.cpp
How about we just inline it into that one test, and kill off the .h file?

libcxx/utils/generate_header_tests.py
59

Please also uncomment the TODO line in libcxx/utils/generate_header_inclusion_tests.py, and re-run it.

ChuanqiXu updated this revision to Diff 378115.Oct 8 2021, 12:47 AM

Address comments. Thanks for the detailed review of @Quuxplusone!
The comments not addressed would be commented separately.

I think I addressed the comments except the following 3:

libcxx/include/coroutine
105

_LIBCPP_HIDE_FROM_ABI
coroutine_handle() = default;

Defaulting it will give us constexpr and noexcept for free.

I use the following form:

_LIBCPP_HIDE_FROM_ABI
constexpr coroutine_handle() noexcept = default;

Since I think this is more direct and clear.

Line 214 above implies that _Promise might be a cv-qualified type; like, you could have a coroutine_handle<const Widget>. Do we need to ask LWG if that's intentional?

It looks natural to me that coroutine_handle<const Widget> is valid. Remind me if there is use case which shows the difference.

  • If so, then static_cast here might not be good enough, right?

To my knowledge, it is invalid to use static_cast to cast away constness. But it should be fine to use static_cast to add constness.

  • If not, then line 214 could be simplified, right?
  • Do we have any tests for coroutine_handle<const Widget>?

We had ''test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.prom/promise.pass.cpp" to test this.

libcxx/test/std/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
32–38

I don't understand this comment enough. It looks like I need to write a second test file which is almost like this one. So it looks like most of the code would be redundant. Did I misunderstand something?

libcxx/test/std/language.support/support.coroutines/lit.local.cfg
7 ↗(On Diff #378115)

(I can't find the original inline comments. I don't know the reasons.)

I vaguely remember we talked about this somewhere; but now I'm skeptical of it. If you simply remove this lit.local.cfg file, what breaks?

If we remove this lit.local.cfg` file, then there would be tests failed in libcxx if we use gcc to compile it. According to our previous discussing, the support for gcc could and should be done in another PR.

http://eel.is/c++draft/coroutine.syn doesn't say anything about having to include <new>.

I'd say delete this test... and then also delete #include <new> from <coroutine>, unless it's actually needed (which it might be).

I guess the reason why <experimental/coroutine> includes <new> is that a coroutine needs ::operator new to allocate memory to store coroutine status. So the original implementation includes it directly.
And I tested <coroutine> without including <new> with our internal projects. It looks fine. I think it might be a break change when people refactor from <experimental/coroutine>. But I guess it might not be a big problem. So I choose to delete #include <new> from <coroutine> as you said.

Some of my comments haven't been addressed (nor marked "Done") yet.
I think the big blocker here is deciding whether we want <coroutine> to be supported pre-C++20.

libcxx/test/std/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
32–38

I don't understand this comment enough. It looks like I need to write a second test file which is almost like this one.

I think you should either write a second test file which is almost the same as this one, or just mark this test UNSUPPORTED: c++14. Right now, this test is hard to understand because the Noisy type has different behavior in C++14 versus C++17/20/2b. My goal is to get rid of the #if ... FIXME stuff, one way or the other.

I think we need some design input from @ldionne: Should the standard <coroutine> header be supported pre-C++20 at all? Right now <experimental/coroutine> is supported all the way back to -std=c++03 -fcoroutines-ts, and also in -std=c++20-and-greater without -fcoroutines-ts.
My inclination is to say "no, <coroutine> should be supported in -std=c++20-and-greater only." In that case, then all of these tests should be marked

// UNSUPPORTED: c++03, c++11, c++14, c++17

and all of their uses of TEST_STD_VER should go away. However, @ChuanqiXu, I'm not sure if @ldionne is going to agree with that direction; I'd want to see what he says.

45

nit: s/return{};/return {};/

libcxx/test/std/language.support/support.coroutines/end.to.end/generator.pass.cpp
36–37

Throughout: Tests should avoid using reserved names. Rename _Coro to coro, _Done to done, etc. (Or coro_, done_, etc., because they're data members; I have no strong opinion here.) Also _Right to right (or rhs), etc.

146–153

Style/organization nit: It feels like minig is completely unrelated to the rest of this test file. Shouldn't we move test_mini_generator and minig into their own test file, named e.g. end.to.end/mini_generator.pass.cpp?

libcxx/test/std/language.support/support.coroutines/end.to.end/go.pass.cpp
163

This leaks? Let's use a unique_ptr or vector for this (and move it into main).

167

Couldn't this line be replaced with simply pusher(c[i], c[i + 1]);? goroutine::go seems to be a no-op and we could just eliminate it.

libcxx/test/std/language.support/support.coroutines/lit.local.cfg
7 ↗(On Diff #378115)

I guess this makes sense, and can be deferred to a later PR.
But I do worry that this means we are not testing this directory in -std=c++20 mode; we're testing it only in -std=c++20 -fcoroutines-ts mode, which may or may not be the same thing (I don't know).
This is another issue that would become moot if @ldionne decides that <coroutine> should be supported only in -std=c++20 mode.

libcxx/include/__coroutine/coroutine_handle.h
2

Throughout: Please remove the name of the file from the copyright-header.
You can (and should) just copy the copyright header from any other existing file.

88

Nit: You can remove the noexcept here, since this function is a private implementation detail. (Getting a private method's noexcept-specifier wrong has bad consequences — it'll terminate the program — but getting it right has no benefit. So it's best to leave it off.)

99–102

If we intend to support <coroutine> pre-C++20, then you should also provide an operator!=, and a test case that exercises it.
If we don't intend to support <coroutine> pre-C++20, then this is OK, and maybe even unnecessary: do we still have any supported targets which claim C++20 but don't support operator<=>?

128

This should use std::compare_three_way()(__x.address(), __y.address()), for the same reason your operator< uses std::less<void*>.
https://eel.is/c++draft/coroutine.handle#compare-2

libcxx/include/__coroutine/coroutine_noop.h
87

Please make sure each file ends with a newline. (Your text editor might have a setting for this.)

libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.capacity/operator_bool.pass.cpp
36–39

Remove lines 35–38. Remove the ((void)c); on line 33.

libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.con/assign.pass.cpp
30

Remove this line.

libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.con/construct.pass.cpp
26–54

I'd rather see this test (and maybe some other similar ones below) as:

template <class C>
constexpr bool do_test() {
  static_assert(std::is_nothrow_constructible<C>::value, "");
  static_assert(std::is_nothrow_constructible<C, std::nullptr_t>::value, "");
  {
    C c;
    assert(c.address() == nullptr, "");
  }
  {
    C c = C(nullptr);
    assert(c.address() == nullptr, "");
  }
  return true;
}

int main(int, char**)
{
  do_test<std::coroutine_handle<>>();
  do_test<std::coroutine_handle<int>>();
  static_assert(do_test<std::coroutine_handle<>>());
  static_assert(do_test<std::coroutine_handle<int>>());

  return 0;
}

Basically, any test involving a constexpr variable could be rewritten this way, to use a constexpr function instead — and then we can test the same function both at constexpr time and at runtime.

libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.hash/hash.pass.cpp
37–40

Do this to fix the FIXME:

LIBCPP_ASSERT(h(LHS) == ExpectLHS);
LIBCPP_ASSERT(h(RHS) == ExpectRHS);
assert((h(LHS) == h(RHS)) == (LHSVal == RHSVal));
50–59

Here and in coroutine.handle.compare/equal_comp.pass.cpp, it would be a lot nicer if we didn't reinterpret_cast integers to pointers. (Then we might even be able to test it in constexpr mode!)
A non-casting version would look something like

int i, j;
std::pair<int*, int*> TestCases[] = {
    { nullptr, nullptr },
    { nullptr, &i },
    { &i, &i },
    { &i, &j },
};
for (auto& TC : TestCases) {
    do_test<std::coroutine_handle<>>(TC.first, TC.second);
    do_test<std::coroutine_handle<int>>(TC.first, TC.second);
}

and then in do_test,

template <class C>
void do_test(int *LHSVal, int *RHSVal) {
  size_t ExpectLHS = std::hash<void*>{}(LHSVal);
  size_t ExpectRHS = std::hash<void*>{}(RHSVal);
  C LHS = C::from_address(LHSVal);
  C RHS = C::from_address(RHSVal);
  const std::hash<C> h;

etc.

ldionne added inline comments.Oct 15 2021, 11:05 AM
libcxx/test/std/language.support/support.coroutines/lit.local.cfg
7 ↗(On Diff #378115)

Thanks for pinging me on Discord about this. We're not going to support coroutines prior to C++20. We shouldn't even try. If people want to use coroutines, they can use -std=c++20, that's why we work so hard to preserve backwards compatibility between standards.

Similarly, any attempt to improve the support we have for <experimental/coroutine> should not be pursued, since we're going to deprecate and then remove it anyways.

ldionne added inline comments.Oct 15 2021, 11:07 AM
libcxx/test/std/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
32–38

I answered the same elsewhere on this review, but for the record, <coroutine> should only be supported starting in C++20.

It's not just my personal preference -- it's what we consistently do for all features in libc++ unless we have an incredibly compelling reason to stray away from that guideline. We've tried supporting stuff in earlier standards in the past and almost the only thing this has done to us - and our users - is harm.

Quuxplusone requested changes to this revision.Oct 15 2021, 11:31 AM
Quuxplusone added inline comments.
libcxx/test/std/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
32–38

Awesome. @ChuanqiXu:

  • Throughout your additions to libcxx/include/, please change #if _LIBCPP_STD_VER > 11 to #if _LIBCPP_STD_VER > 17.
  • Mark all your new tests as UNSUPPORTED: c++03, c++11, c++14, c++17. Eliminate any references to TEST_STD_VER pre-C++20.
  • Now see if you can eliminate that lit.local.cfg file — since in C++20 mode we should not need any special options to enable -fcoroutines-ts support.
This revision now requires changes to proceed.Oct 15 2021, 11:31 AM

I didn't do a review, but noticed some missing updates to our status information.

libcxx/utils/generate_header_tests.py
59

If this review completes coroutines support in C++ it should update a few more files:

  • generate_feature_test_macro_components.py for __cpp_lib_coroutine remove the line "unimplemented": True, and run this script to update the appropriate headers.
  • Mark the LWG-issue 3393 in docs/Status/Cxx20Issues.csv as complete.
  • Mark the other appropriate papers and LWG issue as complete.
ChuanqiXu updated this revision to Diff 380288.Oct 18 2021, 1:01 AM

Address comments from @Quuxplusone and @Mordante. Change includes mainly:

  • Support <coroutine> from C++20 only.
  • Mark all the new tests as UNSUPPORTED: c++03, c++11, c++14, c++17.
  • Eliminate all TEST_STD_VER uses.
  • Eliminate lit.local.cfg in test files.
  • Mark do_test as constexpr as much as possible to make it tested at constexpr time and runtime.
  • Eliminates all reinterpret_cast in tests.
  • Run generate_feature_test_macro_components.py for __cpp_lib_coroutine to update the appropriate headers.
  • Mark the LWG-issue 3393 in docs/Status/Cxx20Issues.csv as complete.

There is another issue 3460 in docs/Status/Cxx2bIssues.csv. It is about noop_coroutine_handle. The current status is that it is OK under clang but it can't work under GCC due to the different implementation strategies. We have decided to support this in successive patches.

I had also tested these tests for gcc10. All of them are passed. (The test for noop_handle is skipped.)

Another change is this diff introduced the feature test macro __cpp_­impl_­coroutine in clang. We could find it at: https://eel.is/c++draft/tab:cpp.predefined.ft. Clang uses __cpp_coroutines before. I wanted to make it a different patch since it touches clang part. But the change is really small. So I remain it in this patch.
I remain the use for __cpp_coroutines for considering compatibility. The macro tests using __cpp_coroutines is renamed to _LIBCPP_LEGACY_HAS_NO_COROUTINES. This is used in <experimental/coroutine> only.

ChuanqiXu marked 42 inline comments as done.Oct 18 2021, 1:23 AM

Mark inline comments as done.

libcxx/include/__coroutine/coroutine_handle.h
99–102

I am note sure if there is targets which claim C++20 but don't support operator<=>. I guess the current solution is conservative and should be find.

libcxx/include/coroutine
272

Line 214 above implies that _Promise might be a cv-qualified type; like, you could have a coroutine_handle<const Widget>. Do we need to ask LWG if that's intentional?

  • If so, then static_cast here might not be good enough, right?
  • If not, then line 214 could be simplified, right?
  • Do we have any tests for coroutine_handle<const Widget>?

Since the contents is removed. I replied this in above.

libcxx/include/experimental/coroutine
300–316

There are constexpr tests in suspend_always.pass.cpp and suspend_never.pass.cpp

libcxx/test/std/language.support/support.coroutines/end.to.end/generator.pass.cpp
146–153

I prefer to remain it in the current test file or remove it directly. Since mini_generator is not a terminology. It is a little bit confusing to move it to new test file.

ChuanqiXu updated this revision to Diff 380305.Oct 18 2021, 1:32 AM
ChuanqiXu marked an inline comment as done.

Address the comments in oneshot_func.pass.cpp.

ChuanqiXu marked an inline comment as done.Oct 18 2021, 1:32 AM

General comment regarding stdlib/compiler compatibility.

I notice that you are keeping the std::experimental::coroutine_handle types but leaving them as separate types rather than making them type-aliases of the std::coroutine_handle types.

This raises, for me, the question of what types the compiler will use under what compilation flags.
The compiler forms references to the stdlib types when lowering coroutine code.
e.g. it builds a reference to a specialisation of std::coroutine_traits to determine the promise_type and builds a reference to a specialisation of std::coroutine_handle when building the await-suspend expression.

If I have code that was previously using std::experimental::coroutine_handle and the compiler is updated to use std::coroutine_handle::from_promise() in its await-suspend expression lowering then this code will fail to compile.
Now maybe, this isn't a big issue as std::experimental code shouldn't be assumed to continue working ongoing anyway.
However, it might also go the other way - I write code using the new std::coroutine_handle but try to compile it against an older compiler that lowers await-suspend expressions to std::experiemental::coroutine_handle<P>::from_promise().
Should such code be made to continue working?
Should there be some kind of inter-compatibility between std::experiemental::coroutine_handle and std::coroutine_handle?
e.g. if compiler uses std::coroutine_handle then allow implicit conversion from std::coroutine_handle to std::experimental::coroutine_handle?
Or if compiler uses std::experimental::coroutine_handle then allow implicit conversion from std::experimental::coroutine_handle to std::coroutine_handle?

Is the intention to just require everyone to move to a clang compiler that supports std::coroutine_handle and abandon support for std::experimental types?
Once the clang compiler has been updated to reference the new types, is there any point in continuing to keep the std::experimental types around?
Just wondering what the strategy should be here.

libcxx/include/__coroutine/coroutine_handle.h
88

I wonder if this check should be done as part of the respective __builtin_coro_xxx() intrinsics (e.g. when UB-san is turned on) rather than as part of the library here.

For the library to be able to check this in general will require additional constraints on the ABI of the coroutine implementation (e.g. to ensure that it writes a certain value to the coroutine-frame in a known location).

This would be required in general for all build modes since we can't know that all coroutines are compiled with the same compiler flags that turns on these checks or not. e.g. an optimised TU may be linked against a debug TU that has this assertion turned on and so needs to access the ABI of the coroutine-frame.

Hi Lewis, thanks for looking into this!

General comment regarding stdlib/compiler compatibility.

I notice that you are keeping the std::experimental::coroutine_handle types but leaving them as separate types rather than making them type-aliases of the std::coroutine_handle types.

Yes. This is required by @ldionne. I think he could give you better answer from the perspective of authority. Here is my understand to your comments.

This raises, for me, the question of what types the compiler will use under what compilation flags.
The compiler forms references to the stdlib types when lowering coroutine code.
e.g. it builds a reference to a specialisation of std::coroutine_traits to determine the promise_type and builds a reference to a specialisation of std::coroutine_handle when building the await-suspend expression.

The patch for the clang side is in D108696, which would be landed before this patch. The answer for your question is that there is no extra compilation flags needed.
The clang would search for std::coroutine_traits (and its specialization) first and in case clang failed, clang would search for std::experimental::coroutine (and its specialization). And clang would complain only after it failed to search std::experimental::coroutine.

If I have code that was previously using std::experimental::coroutine_handle and the compiler is updated to use std::coroutine_handle::from_promise() in its await-suspend expression lowering then this code will fail to compile.
Now maybe, this isn't a big issue as std::experimental code shouldn't be assumed to continue working ongoing anyway.

No, the compiler wouldn't give up to search std::experimental::coroutine_handle immediately. According to D108697, the support for std::experimental::coroutine in libcxx would be abandoned in LLVM15. And the support for std::experimental::coroutine in clang should be continued after that.

However, it might also go the other way - I write code using the new std::coroutine_handle but try to compile it against an older compiler that lowers await-suspend expressions to std::experiemental::coroutine_handle<P>::from_promise().
Should such code be made to continue working?

We couldn't compile code using std::coroutine by older compiler. I think the question Should such code be made to continue working? may be questionable to me. Since the code using std::coroutine should be new code and it shouldn't use continue.

Should there be some kind of inter-compatibility between std::experiemental::coroutine_handle and std::coroutine_handle?

I think the answer above could answer this.

e.g. if compiler uses std::coroutine_handle then allow implicit conversion from std::coroutine_handle to std::experimental::coroutine_handle?
Or if compiler uses std::experimental::coroutine_handle then allow implicit conversion from std::experimental::coroutine_handle to std::coroutine_handle?

I think this is not good. Since it is not the same with the standard and the intention of the patch is to make libcxx/coroutine conforming to the standard as much as we could.
(Although we still leave some TODOs.)

Is the intention to just require everyone to move to a clang compiler that supports std::coroutine_handle and abandon support for std::experimental types?
Once the clang compiler has been updated to reference the new types, is there any point in continuing to keep the std::experimental types around?

Yes, but not immediately. At least the support would continue to LLVM15.

Just wondering what the strategy should be here.

According to @ldionne, this is the continued style of libcxx.

libcxx/include/__coroutine/coroutine_handle.h
88

Reply to lewis's comment:

We could implement `__is_suspended` by adding another bits in the coroutine frame and pay for the cost for an extra store instruction before every suspend. 

To my mind, the intention of this patch is conforming the implementation of libcxx/coroutine to the standard. Since the `__is_suspended` API is not a part of the standard. I think we could ignore `__is_suspended` now. IMO, if we want to support `__is_suspended`, we need to submit another proposal to LWG. Otherwise, we shouldn't implement it directly or we should delete it simply.
ChuanqiXu added a project: Restricted Project.Oct 25 2021, 1:40 AM
ChuanqiXu updated this revision to Diff 382172.Oct 25 2021, 8:25 PM

Run generate_private_header_tests.py and adding the inclusion check for <experimental/coroutine>.

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Right. At the moment CI says "libcxx CI (Format, C++2b, ...) failed". You must fix the "C++2b, ..." parts, but you can leave the "(Format)" part failing. It's sort of like warnings and errors: As long as the CI is red (because of "C++2b, ..." failures), it will also mention that "Format" failed. But, if the only thing that is failing is "Format", then the CI will be green. "Format" is just a warning, not an error.

Fix test ''libcxx/utils/ci/run-buildbot check-generated-output". I also checked generic-cxx2b and generic-cxx20 locally. Hope it could pass CI this time.
Noticed that "libcxx/utils/ci/run-buildbot check-format" would still fail. But I remembered that we shouldn't apply clang-check on libcxx. @Quuxplusone so could we ignore the failed test from libcxx/utils/ci/run-buildbot check-format?

Right. At the moment CI says "libcxx CI (Format, C++2b, ...) failed". You must fix the "C++2b, ..." parts, but you can leave the "(Format)" part failing. It's sort of like warnings and errors: As long as the CI is red (because of "C++2b, ..." failures), it will also mention that "Format" failed. But, if the only thing that is failing is "Format", then the CI will be green. "Format" is just a warning, not an error.

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

D108696 is already correctly listed as a "Parent Revision" of D109433, so D109433's CI is already running on the-combination-of-both-of-them. And it appears that it's still failing. I believe part of the reason is identified in my comments above. I recommend fixing that, also fixing anything that needs fixing in D108696, then doing "Update Diff" on D108696, waiting an hour, doing "Update Diff" on D109433, and then waiting for the CI to complete on both. If that continues to fail CI, then that continues to be a problem (and I would by default assume it's a problem with the patches, not a problem with the CI setup).

libcxx/include/__config
1276

I believe this diff is the reason your tests are failing.
Based on a quick skim, I think the deal is:

  • __cpp_coroutines is set when the compiler supports <experimental/coroutine>
  • __cpp_impl_coroutine is set when the compiler supports <coroutine>

So we might want something here like

#if !(defined(__cpp_coroutines) && __cpp_coroutines >= 201703L)
#define _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES
#endif

#if !(defined(__cpp_impl_coroutine) && __cpp_impl_coroutine >= 201902L)
#define _LIBCPP_HAS_NO_CXX20_COROUTINES
#endif

and then check the appropriate macro in each coroutine header.

This will also need to be reflected in generate_header_tests.py.

libcxx/include/__coroutine/coroutine_handle.h
10

Please rename the include guard to _LIBCPP___COROUTINE_COROUTINE_HANDLE_H.

libcxx/include/__coroutine/coroutine_noop.h
10

Please rename the file to __coroutine/noop_coroutine_handle.h, and the include guard to _LIBCPP___COROUTINE_NOOP_COROUTINE_HANDLE_H.

libcxx/include/__coroutine/coroutine_traits.h
10

Please rename the include guard to _LIBCPP___COROUTINE_COROUTINE_TRAITS_H.

ChuanqiXu added a project: Restricted Project.Oct 29 2021, 2:17 AM
ChuanqiXu updated this revision to Diff 383283.Oct 29 2021, 2:53 AM
ChuanqiXu removed a project: Restricted Project.
  • Rename sub header name and macro as suggested.
  • Move the change part in clang into previous revision D108696.

I suspect the reason why it is failing is that the original version contains the change in clang too! So the CI didn't compile it. Let's see if it could work.

ChuanqiXu marked 2 inline comments as done.Oct 29 2021, 2:57 AM

From what I see in the CI system, the reason why C++2b and C++2a failed is that the compiler wouldn't search in std:: namespace. In another word, the CI system would become green after D108696 checked in.

D108696 is already correctly listed as a "Parent Revision" of D109433, so D109433's CI is already running on the-combination-of-both-of-them. And it appears that it's still failing. I believe part of the reason is identified in my comments above. I recommend fixing that, also fixing anything that needs fixing in D108696, then doing "Update Diff" on D108696, waiting an hour, doing "Update Diff" on D109433, and then waiting for the CI to complete on both. If that continues to fail CI, then that continues to be a problem (and I would by default assume it's a problem with the patches, not a problem with the CI setup).

I guess the reason is that this patch included the change in clang part and it doesn't tag clang. So the change isn't compiled in CI. Now I move the change in clang part into the parent revision. Let's see if it could works : )

libcxx/include/__config
1276

_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES is defined in experimental/__config now. I think the place is better.
And I remain the name _LIBCPP_HAS_NO_COROUTINES instead of _LIBCPP_HAS_NO_CXX20_COROUTINES since it looks a little bit redundancy.

ChuanqiXu updated this revision to Diff 383336.Oct 29 2021, 6:51 AM

Update generated test

ChuanqiXu added a project: Restricted Project.Oct 29 2021, 7:16 AM
libcxx/include/__config
1276

FWIW, I'm okay with "keeping" the name _LIBCPP_HAS_NO_COROUTINES; but I do still think it's better to change all uses of _LIBCPP_HAS_NO_COROUTINES to _LIBCPP_HAS_NO_{CXX20,EXPERIMENTAL}_COROUTINES. Both for programming reasons (so that the future reader can unambiguously see that a certain codepath is related to C++20 or experimental or both, based on the name(s) of the macros involved), and for engineering reasons (so that the patch-writer is forced to touch every instance of _LIBCPP_HAS_NO_COROUTINES in the entire codebase, and any _LIBCPP_HAS_NO_COROUTINES that remains in the codebase can be instantly identified as an oversight, not just something that coincidentally didn't need to be changed).

Anyway, I assume you do intend that one of these macros relates to experimental coroutines and the other one relates specifically to the non-experimental C++20 coroutines. Right?:

#if defined(_LIBCPP_HAS_NO_{CXX20_,}COROUTINES) && defined(_LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES)
// we have no coroutine support at all
#else
// we have _some_ kind of coroutines (maybe C++20, maybe experimental)
#endif

@Quuxplusone for the still failing CI system, I believe it didn't combine D108696. Here is my reason:

So it didn't contain the codes in D108696. So if we care the CI result, we could wait after D108696 merged in.

libcxx/include/__config
1276

Yeah, I am not a defender on the names. Since I am not a native speaker, so I trust your sensitivity on names should be right. I would rename it later.

ChuanqiXu updated this revision to Diff 384657.Nov 3 2021, 9:57 PM

Rename _LIBCPP_HAS_NO_COROUTINES to _LIBCPP_HAS_NO_CXX20_COROUTINES.

Oh, it is weird that the CI keeps failing after landing D108696. I have tested many many times locally. I am wondering if the compiler used in the CI system didn't get updated in time. Let's see if it could be green tomorrow.

I had an experiments in https://reviews.llvm.org/D113170. The diff there is the same with here except that diff would check if the compiler introduces __cpp_impl_coroutine which should be introduced in previous commit: https://github.com/llvm/llvm-project/commit/ec117158a390a0ebf64377caa5abd0c976df8f7a. And the results there shows that the compiler in the CI system didn't contain macro __cpp_impl_coroutine so that's the reason for failing.
I guess it would be updated in days. Let's wait some days.

Quuxplusone requested changes to this revision.Nov 5 2021, 8:52 AM
Quuxplusone added inline comments.
libcxx/test/std/language.support/support.coroutines/coroutine.handle/void_handle.pass.cpp
10

libc++ needs to work on Clang 13.x as well as trunk/14.x. Clang 13.x doesn't define __cpp_impl_coroutine. Therefore, to fix the buildkite, I think you need to introduce a new feature flag so that you can say UNSUPPORTED: libcpp-no-coroutines on all of these new tests. Grep for libcpp-no-concepts and see if the process is self-explanatory... or ask @ldionne what's the right way to do it. :)

This revision now requires changes to proceed.Nov 5 2021, 8:52 AM
ChuanqiXu updated this revision to Diff 385226.Nov 5 2021, 8:32 PM

Disable newly added tests for 13.x by adding libcpp-no-coroutines feature. I have tested 13.x locally. Hope CI could get green this time : (

ChuanqiXu updated this revision to Diff 385234.Nov 5 2021, 10:22 PM

re-check check-generated-output

ChuanqiXu updated this revision to Diff 385244.Nov 6 2021, 1:00 AM
  • Include <version> in coroutine
  • Export compare for coroutine in module map.

I'm not sure I'm reading the test results right, but if I am, then it looks like we're still skipping all your new tests — the test runner thinks that Clang 14.x doesn't support __cpp_impl_coroutine.
https://buildkite.com/llvm-project/libcxx-ci/builds/6457
But I have verified locally that Clang trunk (14.x) clang++ -std=c++20 -dM -E x.cpp does output #define __cpp_impl_coroutine 201902L, so I think this is just due to buildkite using an old build... I'm not sure. We still shouldn't land this until we can prove that buildkite will run (and pass) these new tests.

libcxx/utils/generate_header_tests.py
58–60

I think we can now change these lines to

"experimental/coroutine": ["ifndef _LIBCPP_HAS_NO_EXPERIMENTAL_COROUTINES"],
"coroutine": ["ifndef _LIBCPP_HAS_NO_CXX20_COROUTINES"],

for better consistency with all the other features in this list.

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

ChuanqiXu updated this revision to Diff 385386.Nov 7 2021, 6:10 PM

Address comments.

ChuanqiXu marked an inline comment as done.Nov 7 2021, 9:44 PM

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

General comment regarding stdlib/compiler compatibility.

I notice that you are keeping the std::experimental::coroutine_handle types but leaving them as separate types rather than making them type-aliases of the std::coroutine_handle types.

As it has been explained by @ChuanqiXu , the plan is to first add support for C++20 coroutines while keeping around experimental coroutines for two releases. After two releases, we remove the experimental ones and any user that hasn't migrated yet will be broken. This is the policy we have for experimental features in libc++, which tries to balance keeping the library sane without being too eager to break users.

Is the intention to just require everyone to move to a clang compiler that supports std::coroutine_handle and abandon support for std::experimental types?

Yes, but after two releases. During this two-release time frame, both experimental and non-experimental coroutines should keep working. We probably don't want to allow mixing the two for sanity, though. My understanding is that this is what D108696 achieves.

Once the clang compiler has been updated to reference the new types, is there any point in continuing to keep the std::experimental types around?

Just to avoid breaking users too eagerly. We want to give them a bit of time to migrate, but then we should nuke std::experimental as soon as we can to avoid user confusion and reduce our maintenance costs.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Long story short, our CI doesn't get an updated version of the compiler on apt.llvm.org automatically. They cache the Docker image and don't rebuild it unless the Dockerfile changes AFAICT. This isn't great -- we should always use the Dockerfile in the repo to run the build, but there are hurdles to setting that up. I'll manually change the Dockerfile to trigger an image rebuild -- we should pick up the latest apt.llvm.org compiler, which I think should include D108696.

By the way, thanks a lot to everyone for chiming in and reviewing this. I've been MIA because I'm swamped by other work, but please keep pinging me on Discord for specific questions.

ChuanqiXu updated this revision to Diff 385685.Nov 8 2021, 7:25 PM

Rebase with trunk.

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 8 2021, 7:25 PM

The build system uses https://apt.llvm.org/ for its packages. This isn't updated nightly so it might be the build used an old version. The current timestamp of the build is 20211106103030. @ChuanqiXu can you rebase this patch to trigger the CI again?

@Mordante Due to my system is not Debian/Ubuntu, I couldn't test the package in the link locally. And I sent another diff in: https://reviews.llvm.org/D113170. It is the same with this one except that it would check the macro 'cpp_impl_coroutine'. And the result shows that the compiler in the CI still didn't emit macro 'cpp_impl_coroutine'. Could you help to check the compiler in the CI system or tell me how to test the timestamp of the compiler in the CI system? Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Got it. Thanks.

I run Debian so I tested locally which version was available in apt.llvm.org. I just did a quick test with the Docker image we run on our CI. There the version installed is of the 27th of October. When I manually update in Docker I get a binary of the 5th of November. (Not sure why Ubuntu has a one day older build than Debian). So it seems our CI uses a version of Clang before D108696 landed. I have some suspicions, but @ldionne knows more about our CI.

Long story short, our CI doesn't get an updated version of the compiler on apt.llvm.org automatically. They cache the Docker image and don't rebuild it unless the Dockerfile changes AFAICT. This isn't great -- we should always use the Dockerfile in the repo to run the build, but there are hurdles to setting that up. I'll manually change the Dockerfile to trigger an image rebuild -- we should pick up the latest apt.llvm.org compiler, which I think should include D108696.

It looks like that the CI didn't update the compiler yet. May I ask when would it be OK to use a later version?

ChuanqiXu updated this revision to Diff 386095.Nov 10 2021, 1:50 AM

Now it shows that the compiler should be updated. And there are two tests failing:

  • libcxx/utils/ci/run-buildbot generic-modules
  • libcxx/utils/ci/run-buildbot generic-32bit
    • libcxx/gdb/gdb_pretty_printer_test.sh.cpp

I don't find the reason why generic-modules fails now. And it is weird that libcxx/gdb/gdb_pretty_printer_test.sh.cpp is failing since it is irrelevant with coroutine. I would test them locally.

ChuanqiXu updated this revision to Diff 386120.Nov 10 2021, 4:19 AM

Fix the test failure for generic modules.
I finally found the reason why it would fail for generic modules. Since the tests didn't contain headers which is included in the __coroutine/* headers even if they are already included! It is surprising to people who didn't use clang module before. Then the only failure is for libcxx/gdb/gdb_pretty_printer_test.sh.cpp. It looks not relevant with coroutines. But I would try to look into it in case it would fail still.

When I look into the error message, I find the following error message:

FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:247
GDB printed:
   'std::bitset<258u>'
Value should match:
   'std::bitset<258(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:250
GDB printed:
   'std::bitset<0u>'
Value should match:
   'std::bitset<0(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:253
GDB printed:
   'std::bitset<15u> = {[2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, [7] = 1, [8] = 1, [9] = 1}'
Value should match:
   'std::bitset<15(ul)?> = {\\[2\\] = 1, \\[3\\] = 1, \\[4\\] = 1, \\[5\\] = 1, \\[6\\] = 1, \\[7\\] = 1, \\[8\\] = 1, \\[9\\] = 1}'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/344e6285b245-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:261
GDB printed:
   'std::bitset<258u> = {[0] = 1, [129] = 1, [132] = 1}'
Value should match:
   'std::bitset<258(ul)?> = {\\[0\\] = 1, \\[129\\] = 1, \\[132\\] = 1}'

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

[gdb_pretty_printer_test.sh.cpp is failing]

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

  • I'm 100% sure that the GDB-pretty-printer test failure is not your fault.
  • I think @ldionne should look into it (in fact, my impression is that he already is). Maybe the solution will just be to UNSUPPORTED it temporarily, but let's actually get it green.
  • IMO this coroutines stuff is big and important enough that you should wait until that pretty-printer test has been fixed, and get a really 100% green CI run, before you land it. It's been a few months; waiting another few days won't hurt anything. And green CI runs for big features are nice to have. So I don't think you should land it until CI is green.

[gdb_pretty_printer_test.sh.cpp is failing]

It looks like this is introduced in https://reviews.llvm.org/D113112. @ldionne hi, could you help to look into this? @Quuxplusone Now I think this test failure should be irrelevant with this diff and all other tests are passed. Do you think we should land this after the bug fixed or we could land this now?

  • I'm 100% sure that the GDB-pretty-printer test failure is not your fault.
  • I think @ldionne should look into it (in fact, my impression is that he already is). Maybe the solution will just be to UNSUPPORTED it temporarily, but let's actually get it green.
  • IMO this coroutines stuff is big and important enough that you should wait until that pretty-printer test has been fixed, and get a really 100% green CI run, before you land it. It's been a few months; waiting another few days won't hurt anything. And green CI runs for big features are nice to have. So I don't think you should land it until CI is green.

Got it. Thanks for the explanation!

@Quuxplusone the CI is finally green now!

ldionne accepted this revision.Nov 15 2021, 2:16 PM

LGTM with some suggestions. I am assuming that you copy-pasted the code and tests from experimental/coroutine and performed minor modifications as explained in the description.

Thanks!

libcxx/include/CMakeLists.txt
134–137

Please sort!

libcxx/include/__coroutine/coroutine_handle.h
98

I *think* this block can be removed entirely, but let's not block this on D113938.

libcxx/include/__coroutine/noop_coroutine_handle.h
23 ↗(On Diff #386120)

Can we get rid of this (and similarly in the corresponding test file)?

libcxx/include/CMakeLists.txt
134–137

And before sorting, please rename coroutine_trivial_awaitables to just trivial_awaitables (and update the modulemap, and the header guard, and the generated tests). I just noticed this. :P

ChuanqiXu updated this revision to Diff 387467.Nov 15 2021, 6:21 PM

Rename coroutine_trivial_awaitables to trivial_awaitables and sort CMakeLists, modulemap .
(Didn't change the header guard since I found it is consistency now)

ChuanqiXu marked 2 inline comments as done.Nov 15 2021, 6:24 PM
ChuanqiXu added inline comments.
libcxx/include/__coroutine/noop_coroutine_handle.h
23 ↗(On Diff #386120)

Only Clang and MSVC implements __builtin_coro_noop. GCC uses other tricks. So if we move get rid of this guard, it couldn't compiled by GCC. According to our previous discussion, we decide to support this in later patches.

ChuanqiXu updated this revision to Diff 387481.Nov 15 2021, 7:36 PM

Rename correctly.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 15 2021, 10:15 PM
This revision was automatically updated to reflect the committed changes.

@Quuxplusone @ldionne many thanks for your helps! I couldn't make this without your helps.