[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov abandoned this revision. ilya-biryukov added a comment. Marking the revision as abandoned to reflect the status of the RFC. Thanks everyone for your feedback and sorry about the timing of this change again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

jyknight added a comment. As I posted on the RFC thread, I think we have a viable alternative solution to address the original motiving use-case. One might potentially still make a case for implementing the `-fno-coroutines` flag for GCC compatibility, but given the concerns raised -- and that

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a comment. In D156247#4542769 , @ilya-biryukov wrote: > In D156247#4542155 , @aaron.ballman > wrote: > >> Given the views expressed on this thread, I think this requires a wider >> community

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4542155 , @aaron.ballman wrote: > Given the views expressed on this thread, I think this requires a wider > community discussion to determine whether we want to support this idea or > not, regardless of -cc1 or

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

Mordante added a comment. In D156247#4541756 , @ilya-biryukov wrote: > In D156247#4540228 , @Mordante > wrote: > >> 2. My biggest concern is the timing of this patch. The date of the branching >> is know upfron

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a comment. Given the views expressed on this thread, I think this requires a wider community discussion to determine whether we want to support this idea or not, regardless of -cc1 or driver-level option. We should not be adding a novel language dialect as we're getting read

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4541826 , @cor3ntin wrote: > How does a -cc1 flag alleviate the burden concerns for libc++? It would still > be something they would have to support, unless we plan to remove the flag > before c++26 and you are

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

Endill added a comment. In D156247#4541756 , @ilya-biryukov wrote: > @aaron.ballman the internal -cc1 flag > > - internal `clang -cc1 -fno-coroutines`. I'm sorry, but I find this wording misleading. `-cc1 -fno-coroutines` is not internal in the same se

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

cor3ntin added a comment. How does a -cc1 flag alleviate the burden concerns for libc++? It would still be something they would have to support, unless we plan to remove the flag before c++26 and you are happy with the inclusion of `` and `` producing subpar errors. However, in the C++26 cycle,

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added subscribers: thieta, tstellar. ilya-biryukov added a comment. @aaron.ballman the internal `-cc1` flag is good enough for us and should aleviate most of the concerns in this thread. We could also live without a libc++ change. I suggest to move forward with `-cc1` flag. Would t

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. > We should reach out to some GCC folks to see if this is an oversight in their > documentation or not. I've sent a mail: https://gcc.gnu.org/pipermail/gcc/2023-July/242159.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

Mordante added a comment. In D156247#4538626 , @aaron.ballman wrote: > I would like explicit buy-in from the libc++ folks on the idea of adding > `-fno-coroutines` as they're going to be impacted by Clang supporting such an > option (and they may have

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a comment. In D156247#4538664 , @ilya-biryukov wrote: > Thanks, disabling the language feature definitely makes more sense. > `-fno-coroutines` is trickier to implement, but this cannot be too hard. > > While we are waiting for libc++

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4538962 , @philnik wrote: > I don't think we want to support having another flag for a dialect, since > that will result in more problems down the road. We already have a problem > with the number of configurati

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov updated this revision to Diff 544783. ilya-biryukov added a comment. Herald added a project: clang-format. Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay. - Expose -fno-coroutine and add tests Current implementation exposes: - `-fno-coroutines` to disab

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

philnik added a comment. In D156247#4538626 , @aaron.ballman wrote: > I would like explicit buy-in from the libc++ folks on the idea of adding > `-fno-coroutines` as they're going to be impacted by Clang supporting such an > option (and they may have a

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a subscriber: ldionne. ilya-biryukov added a comment. In D156247#4538647 , @cor3ntin wrote: > Given that adoption by Apple Clang is one of the motivation to make this > change upstream, do we have a reasonable expectation they would p

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. Thanks, disabling the language feature definitely makes more sense. `-fno-coroutines` is trickier to implement, but this cannot be too hard. While we are waiting for libc++ folks to come back with feedback, what would be the desired behavior. GCC seems to 1. error

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

cor3ntin added a comment. In D156247#4534645 , @ilya-biryukov wrote: > At Google, we would like to postpone adoption of coroutines until the > aforementioned bugs are fixed, but we feel that Clang 17 would be a good > candidate to enable other C++20 fe

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. Thanks for the explanation. I am strongly in favor with you : ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156247/new/ https://reviews.llvm.org/D156247 ___ cfe-commits maili

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a reviewer: libc++. aaron.ballman added a comment. I would like explicit buy-in from the libc++ folks on the idea of adding `-fno-coroutines` as they're going to be impacted by Clang supporting such an option (and they may have additional testing requirements to ensure that l

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a comment. In D156247#4537351 , @ChuanqiXu wrote: > While I am not against the approach, do you think we need similar semantics > for `-fno-concepts`, `-fno-modules`, etc... ? I think we need to take it on a case by case basis. In ge

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. FYI, for https://github.com/llvm/llvm-project/issues/56301, I've posted https://discourse.llvm.org/t/rfc-a-new-aapass-for-coroutines-or-a-simple-workaround/72336 to make some initial progress. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. While I am not against the approach, do you think we need similar semantics for `-fno-concepts`, `-fno-modules`, etc... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156247/new/ https://reviews.llvm.org/D156247

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

shafik added a comment. In D156247#4536034 , @tahonermann wrote: >> You are absolutely right, -fno-coroutines would totally work for us, had it >> been available. > > Good. Gcc handles `-fcoroutines` and `-fno-coroutines` as I would expect > (https://g

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a comment. In D156247#4536034 , @tahonermann wrote: >> You are absolutely right, -fno-coroutines would totally work for us, had it >> been available. > > Good. Gcc handles `-fcoroutines` and `-fno-coroutines` as I would expect > (ht

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. > You are absolutely right, -fno-coroutines would totally work for us, had it > been available. Good. Gcc handles `-fcoroutines` and `-fno-coroutines` as I would expect (h

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

cor3ntin added a comment. If there is consensus we need to do _something_, I would prefer that solution as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156247/new/ https://reviews.llvm.org/D156247 ___

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4535664 , @tahonermann wrote: > At first, it sounded to me like the option that would best suit Google's > needs is `-fno-coroutines`. However: > > - I see that Google does have some limited use of coroutines; p

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

tahonermann added a comment. At first, it sounded to me like the option that would best suit Google's needs is `-fno-coroutines`. However: - I see that Google does have some limited use of coroutines; perhaps that could be enabled on a per-TU or project basis by the build system? However... - T

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. The list of bugs I shared was meant to illustrate the problem, it's not meant to be exhaustive. I am not sure there is a need to rush and fix them before the release, it is just a signal that makes us worried about the feature. The bugs are niche, and likely not a

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a subscriber: rsmith. ChuanqiXu added a comment. There are several topics now. Let's discuss them separately to make things clear: (1) Should we add such warning? > Does the use-case mentioned above make sense (allow C++20, disallow > coroutines for code using Clang 17)? This

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

cor3ntin added a comment. In D156247#4534645 , @ilya-biryukov wrote: > Thanks for the quick feedback! > I understand that this goes against common practice in Clang, but the reason > we want this warning is very practical. > > At Google, we would like t

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. Thanks for the quick feedback! I understand that this goes against common practice in Clang, but the reason we want this warning is very practical. At Google, we would like to postpone adoption of coroutines until the aforementioned bugs are fixed, but we feel tha

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ChuanqiXu added a comment. > however, if there are important unresolved issues, why did we set > __cpp_impl_coroutine ? The discussion is here: https://discourse.llvm.org/t/rfc-could-we-mark-coroutines-as-unreleased-now/69220. My short summary may be: (1) It is not necessary to claim a feature

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:308 def : DiagGroup<"c++98-c++11-c++14-c++17-compat", [CXXPre20Compat]>; +def CXXPre20CompatCoroutines : DiagGroup<"pre-c++20-compat-coro

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

cor3ntin added a comment. We usually reserve these warnings for feature backported to older c++ standards, and I'm not sure "you are using a standard feature" is in general a useful warning. however, if there are important unresolved issues, why did we set `__cpp_impl_coroutine` ? Repository:

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. I know this is a little late and we should have thought about it before cutting Clang 17 release. The plan is to cherry-pick this change into Clang 17 after it lands. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov created this revision. Herald added a subscriber: ChuanqiXu. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang 17 will land with unaddressed coroutine bugs, see - https://github.