We've been shipping <coroutine> since LLVM 14, so LLVM 17 won't ship
the <experimental/coroutine> header per our policy for removing TSes.
Details
- Reviewers
rjmccall lxfind junparser GorNishanov • Quuxplusone ChuanqiXu nicolasvasilache ldionne - Group Reviewers
Restricted Project - Commits
- rG226c444b3882: [libc++] Remove <experimental/coroutine>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
My guess is you might need to keep coro in both std and std::experimental for a while, to allow smooth transition.
- Remain <experimental/coroutine>
- Emit a warning told the user to include <coroutine> if user included <experimental/coroutine>
Yes, it wouldn't compile. So in this revision, I remained the <experimental/coroutine> and add a warning directive to notice the user to include <coroutine>.
Now the user codes should be able to compile all the way after the user update either clang or libcxx. The only exception is that the user uses -Werror. It would the codes couldn't compile after updating. But I think we can't do nothing better.
Yes. I considered that clang and libcxx are two different projects. So the user might update them separately. e.g., update clang only or update libcxx only. So I choose to put warning in both clang part and libcxx part. It may produce redundant warnings if the user update clang and libcxx together. But I thought that it might not be a problem.
First pass. This is still presumably a step in the right direction, but it's currently far from being "C++20 conforming" and IMHO it would probably be nice to get it conforming (or close to conforming) before it lands.
libcxx/include/coroutine | ||
---|---|---|
47–48 ↗ | (On Diff #368816) | Now that we're doing granular headers, please prefer to #include just the smallest helper headers possible (e.g. in this case I think all you need is <__functional/hash.h> for hash<T*>). |
58–60 ↗ | (On Diff #368816) | Remove experimental/ |
86 ↗ | (On Diff #368816) | (1) I'd prefer to see __handle_ get a default member initializer on its declaration, so we don't have to member-initialize it here nor on line 89. (2) If this codepath were limited to _LIBCPP_STD_VER > 11, then we could use constexpr throughout instead of _LIBCPP_CONSTEXPR, and noexcept instead of _NOEXCEPT. Can we do that? Should we? |
107 ↗ | (On Diff #368816) | void resume() const. |
108 ↗ | (On Diff #368816) | Grammar nit: resume() can be called only on suspended coroutines |
114 ↗ | (On Diff #368816) | void destroy() const |
127 ↗ | (On Diff #368816) | This should be marked constexpr. |
133–135 ↗ | (On Diff #368816) | Yes, it must be allowed, at least according to https://eel.is/c++draft/coroutine.handle.export.import ; but you shouldn't have this overload. Lines 126-131 above are already doing the right thing for both null and non-null pointers. |
137–141 ↗ | (On Diff #368816) | This overload looks wrong; remove it. And add a test; something like this: std::coroutine_handle<> h1 = [...]; int *p = (int*)h1.to_address(); std::coroutine_handle<> h2 = std::coroutine_handle<>::from_address(p); |
154 ↗ | (On Diff #368816) | Use stablenames throughout: e.g., // [coroutine.handle.compare] |
155–160 ↗ | (On Diff #368816) | Question for experts: Are we allowed to make this operator a hidden friend? If we're allowed to, then I strongly think we should make it a hidden friend. Ditto, C++20 specifies that operator<=> exists (and I'd like it to be a hidden friend), not the four < <= > >= operators. |
179–185 ↗ | (On Diff #368816) | Here we could just do _LIBCPP_HIDE_FROM_ABI coroutine_handle() noexcept = default; _LIBCPP_HIDE_FROM_ABI coroutine_handle(nullptr_t) noexcept {} because default-constructing a _Base already has the right behavior. Right? |
205–222 ↗ | (On Diff #368816) | Remove all of this. |
libcxx/include/coroutine | ||
---|---|---|
238 ↗ | (On Diff #368816) | This inheritance relationship shouldn't be here. |
285–290 ↗ | (On Diff #368816) | Nits: template <class _Tp> struct hash<coroutine_handle<_Tp> > { _LIBCPP_HIDE_FROM_ABI size_t operator()(const coroutine_handle<_Tp>& __v) const noexcept { return hash<void*>()(__v.address()); } }; |
296 ↗ | (On Diff #368816) | Nit: // not /* (compare to some other header for consistency) |
libcxx/test/libcxx/experimental/language.support/support.coroutines/version.pass.cpp | ||
20 | This file is autogenerated; you shouldn't change its whitespace. | |
libcxx/test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.compare/less_comp.pass.cpp | ||
55 | Undo this whitespace change. | |
libcxx/test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.export/from_address.fail.cpp | ||
30–33 | This test will have to change, since all of these are conforming C++20 and shouldn't be erroring. | |
libcxx/test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.hash/hash.pass.cpp | ||
51–52 | You added some extra spaces here; remove them. (Ditto in several other test files.) | |
libcxx/utils/generate_header_tests.py | ||
9–10 | a-z plz | |
llvm/utils/gn/secondary/libcxx/include/BUILD.gn | ||
362 ↗ | (On Diff #368816) | a-z plz; but also, what is this file? I've never seen it before and we (libc++) definitely haven't been maintaining it. For example, it's missing headers like "concepts" and "compare" and "ranges". But it has "semaphore", so it's not just C++17 stuff... What is this? |
Many thanks for the detailed review! It looks like we need to refactor the implementation for <coroutine> from your comments. I would try to send another patch if possible.
Then it looks like that there is a phase called 'confirming' for c++ features, right? What's that and what does that mean? Is there any documentation?
Then it looks like that there is a phase called 'confirming' for c++ features, right?
No, I said "conforming," as in "make sure our implementation is conforming to the ISO C++20 Standard."
The documentation/specification of what <coroutine> should look like is primarily the standard:
http://eel.is/c++draft/support.coroutine
and secondarily (but sometimes more user-friendly-ly) cppreference:
https://en.cppreference.com/w/cpp/header/coroutine
As for the steps or phases of the political process here, there's only one step: get @ldionne to mark this PR as "accepted." :)
Got it. Although I didn't touch libcxx before, I am willing to refactor this. Thanks : )
As I explained in D109433, we don't need to "move" the header. What we normally do is introduce the <coroutine> header that contains the C++20 conforming implementation without touching <experimental/coroutine>. Then, after 2 releases (LLVM 16), we remove <experimental/coroutine> altogether. I suggest that
- D109433 is used to check-in a conforming <coroutine> implementation, and
- This patch is repurposed to remove <experimental/coroutine> altogether. Once that's the case, this patch can sleep for two releases and I'll rebase + merge it as soon as LLVM 15 is shipped (to make sure it makes it into LLVM 16).
Thanks for repurposing the patch. I'm going to commandeer it to make a few tweaks and I'll keep an eye on this to ship it once we've shipped <coroutine> for two releases.
Things I've done to your patch (just so you know for the future):
- Run libcxx-generate-files. More generally, please look at https://libcxx.llvm.org/Contributing.html.
- Remove <experimental/coroutine> from the module map.
- Don't mention <coroutine> in libcxx/utils/generate_header_tests.py -- that belongs to the patch that will add <coroutine> proper.
@ldionne hi, @Quuxplusone mentioned that <coroutine> would only work under C++20, while there are people who use C++17/C++14 and -fcoroutines-ts combination in practice now. It implies that they have to upgrade to c++20 if they update the libcxx with <experimental/coroutine> removed. I am wondering if this is OK? Or do we need to remove <experimental/coroutine> later? (I guess it would be really long time until the primary user update to C++20.)
We have a policy of removing TSes two releases after we ship the "real" feature. If someone has been writing production code against something in std::experimental, they need to be ready to upgrade. So, generally speaking, my stance here would be that these users indeed need to move to C++20 an adopt the real feature. In practice, we can try to accommodate some users by keeping it around for a bit longer, but I wouldn't do that unless someone explicitly requests it and can provide a justification + timeline for moving off. At the end of the day, keeping the experimental version around is not good for anyone, it just delays work that needs to happen nonetheless.
Also note that one thing we should look into is guarding uses of any experimental feature with some compiler flag like -fexperimental, which would be documented to make it clear that production code is not to be written with that flag enabled. That would make the contract between us and our users more clear.
Maybe we need to land this soon since the release/15.x branch would be created in July 26: https://discourse.llvm.org/t/llvm-15-0-0-release-schedule/63495
You're one release early. Your coroutine patch was part of LLVM 14, so <experimental/coroutine> stays for 14 and 15. It will be removed during the LLVM 16 timeframe.
@aaron.ballman I seem to recall that we had spoken about delaying this for one more release while Clang was deprecating its -fcoroutines-ts flag. Do you remember that or did the holidays mess with my brain?
Otherwise, I guess the next step here would be to post an announcement on Discourse and land this -- but I think it makes sense to coordinate with Clang since that will essentially break the -fcoroutines-ts flag.
Clang haven't started to deprecate -fcoroutines-ts. We plan to deprecate -fcoroutines-ts. The discussion is in https://github.com/llvm/llvm-project/issues/59110. And the discussion looks not related to <experimental/coroutine>. So I think it is OK to remove the <experimental/coroutine> as expected now. Then we can add a warning for -fcoroutines-ts in clang otherwise we might need to do some additional works for the test of <experimental/coroutine>.
We talked about removing -fcoroutines-ts for Clang 16 in https://github.com/llvm/llvm-project/issues/59110, but we didn't really talk about the experimental header except to say that some folks want to set the expectation experimental headers can be removed at any time. Personally, I think we want to be more user-friendly and give users at least a one release window to react to changes (this makes upgrading versions of Clang a bit easier because the problem to be fixed isn't a stop-the-world issue that prevents you from finding further surprises). To me, that should apply to command line flags as well as header files because removing either one causes a stop-the-world problem. Would it make sense to have one libc++ release with a deprecation warning when you include the header along with the command line flag deprecation warning? Or are you ready to be done with this header? :-)
(Speaking of the command line flag deprecation warning, is that something you're working on @ChuanqiXu? If not, I can put something together pretty quickly before the Clang 16 branch.)
To me, that should apply to command line flags as well as header files because removing either one causes a stop-the-world problem.
It looks not bad to me to remove the header files first and keep -fcoroutines-ts flag a little bit longer. Do I misunderstand anything?
(Speaking of the command line flag deprecation warning, is that something you're working on @ChuanqiXu? If not, I can put something together pretty quickly before the Clang 16 branch.)
I haven't started to work on it. I just saw no one else is assigned and it is going to be branched. So that I assign myself to offer some labor help. Feel free to take it you would love to : )
If the user's source code was looking for that header file, they now get a *fatal* error instead of a regular error (so all further compilation stops for that TU): https://godbolt.org/z/zzYEc46fW -- I don't see what value the command line flag adds without the header file, so maybe I'm missing something?
(Speaking of the command line flag deprecation warning, is that something you're working on @ChuanqiXu? If not, I can put something together pretty quickly before the Clang 16 branch.)
I haven't started to work on it. I just saw no one else is assigned and it is going to be branched. So that I assign myself to offer some labor help. Feel free to take it you would love to : )
I'd appreciate it if you can tackle it if you have the chance in the next week or so. I'm still trying to get caught up on reviews (just in time for the C standards meetings in a a few weeks, lol).
Yeah, it is not meaningless to have the command line option without the header file. Since many users would use clang + libstdc++. And there is no such file in libstdc++. So users would use a hacked local "standard" header file which implements something similar to <experimental/coroutine>. Here is an example: https://github.com/scylladb/seastar/blob/master/include/seastar/core/std-coroutine.hh. I know other such uses. I understand that such uses are not encouraged and it is out of the ecosystem of the big LLVM project. I just want to say that there are users who use -fcoroutines-ts without the <experimental/coroutine> in libcxx. I feel they will feel better to receive a warning for deprecating -fcoroutines-ts at first.
(Speaking of the command line flag deprecation warning, is that something you're working on @ChuanqiXu? If not, I can put something together pretty quickly before the Clang 16 branch.)
I haven't started to work on it. I just saw no one else is assigned and it is going to be branched. So that I assign myself to offer some labor help. Feel free to take it you would love to : )
I'd appreciate it if you can tackle it if you have the chance in the next week or so. I'm still trying to get caught up on reviews (just in time for the C standards meetings in a a few weeks, lol).
Got it. I'll try to make it and I'll try to note you to take it if I can't make it due to I'll take a vacation in 20th.
I'd suggest we deprecate the flag and the header in LLVM 16, and remove the header and the flag in LLVM 17.
So we'd hold off with this patch until after the LLVM 16 branch (delaying by one more release).
SGTM! Thank you for your patience while we figure this out. :-)
Thanks, I'm happy to take it over if you can't get to it before your vacation.
@aaron.ballman Can you confirm that you are OK with shipping this now from the Clang point of view? I'm looking forward to stop carrying this review around, rebases are a bit tedious for something touching that many files!
Yup, happy to see it ripped out now (I would be surprised if we get sufficient feedback on the RCs to warrant resurrecting it again, but I suppose that is a possibility).
a-z plz