According to the discussion in https://discourse.llvm.org/t/rfc-could-we-mark-coroutines-as-unreleased-now/69220
We should mark coroutines as "it’s supported fully everywhere but on Windows targets".
Differential D146187
[docs] Update the status for coroutines ChuanqiXu on Mar 15 2023, 8:03 PM. Authored by
Details
According to the discussion in https://discourse.llvm.org/t/rfc-could-we-mark-coroutines-as-unreleased-now/69220 We should mark coroutines as "it’s supported fully everywhere but on Windows targets".
Diff Detail Event TimelineComment Actions Should we also be updating InitPreprocessor.cpp at the same time, for non-Windows targets? I see we define: // TS features. if (LangOpts.Coroutines) Builder.defineMacro("__cpp_coroutines", "201703L"); but we don't define __cpp_impl_coroutine: http://eel.is/c++draft/tab:cpp.predefined.ft And this should definitely come with a release note so users know about the change in status.
Comment Actions
We defined __cpp_impl_coroutine. And __cpp_coroutines is for Coroutines TS. I forgot to remove this too. I'll handle it in a separate patch. Comment Actions
I didn't address this since we didn't do this before. (Defining feature macro according to the targets). Also I feel it may be bad for windows users. Since currently the coroutines on windows is not completely broken. It is still workable in some (or a lot?) situations. It is just out of maintenance. Comment Actions Thanks for handling the TS bits, but what am I misunderstanding here: https://godbolt.org/z/KdrM713r5 ? Comment Actions Hahaha. Comment Actions The problem is, "out of maintenance" basically means "not supported" as far as users are concerned when they run into these bugs. I don't think we should claim to support the feature on a target where we know it has significant issues. I think in general we want feature testing macros to be treated as our signal to the user that we think something is complete, not that we think something is in progress but pretty usable. Comment Actions 😱😱😱 Wow, thank you, that was not something I'd have figured out on my own for quite a while! Ugh, that does sort of change the calculus for whether we do or don't claim support on Windows. If removing the definition of that macro on Windows causes significant code breakage, that would be a reason we should leave it defined. But do we have evidence of that? Comment Actions
+1 Comment Actions
Ah, yes. This is what I want to say. Simply, if we remove the definition for __cpp_impl_coroutines, the coroutine header in MS/STL and libstdc++ can't work anymore. For libstdc++, all the coroutine things are only defined if __cpp_impl_coroutines is defined (https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/coroutine#L58-L69). And this is the same for MS/STL: https://github.com/microsoft/STL/blob/5116678ea11245b232a6e43824aad25fb60b9c6e/stl/inc/yvals_core.h#L1587-L1589 and https://github.com/microsoft/STL/blob/be29af22c049774ae93f2c33e4eb5d801e12216c/stl/inc/coroutine#L17-L39. So I can image that many uses of coroutines in Windows may get useless after we undefined the __cpp_impl_coroutines macros. Comment Actions Okay, that pretty much paints us into a corner, I guess. If we don't define it any longer, we break existing (working) uses of the feature on Windows, but we defined it prematurely. In this case, let's leave the macro defined so we don't break existing uses -- in the future, I think we should be more conservative with defining feature test macros. Comment Actions
Totally agreed. Thanks for your reviewing! |