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

[Ilya Biryukov via Phabricator via cfe-commits]

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 that Clang 17 would be a good candidate to enable other C++20 features (sans Modules).
We could have landed a local patch with a similar warning in our main toolchain that we control tightly. However, we want to have it for other toolchains (e.g. Apple Clang) that are based on upstream and we do not control.

Some code is compiled both by the main toolchain and Apple Clang and we want to have a way to enforce in the compiler that this code does not use coroutines, but is still compiled as C++20.
We are taking a very cautious approach here (the bugs above are rare), but we feel it is warranted given the amount of C++ users that we have.
I do not propose to have flags like this for every single feature, but for pragmatic reasons I think it makes sense to have it for coroutines in Clang 17.

In D156247#4532478 <https://reviews.llvm.org/D156247#4532478>, @cor3ntin wrote:

> 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.

I agree that is weird, but it's not unprecedented: designated initializers are exactly like that <https://gcc.godbolt.org/z/79KhccGj5>.
Maybe there is a different warning wording and flag that would suit this better?

In D156247#4533848 <https://reviews.llvm.org/D156247#4533848>, @ChuanqiXu wrote:

>> 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 is completed only after there is no related open issues. For example, GCC claims coroutines (and many other features) as completed in the early stage and there are still many following bug reports then. 
> (2) Coroutines are used widely and deeply around the C++ world. People from Meta (Meta should be, at least, one of the most heavy user of coroutines in the past years) said they would love to approve to the idea even if it is not bug free.

I also feel this is the right call, coroutines are very much usable and we have been experimenting with them at Google for a more than a year too.
However, scale matters. Our stance is that bugs above are not blockers for one or two projects with experts that can deal with compiler bugs, but they are blockers for allowing all our C++ users to use coroutines.

A few questions for the reviewers to understand how to move forward:

- Does the use-case mentioned above makes sense (allow C++20, disallow coroutines for code using Clang 17)?
- Is the warning on coroutines a proper way to solve this in Clang or are there alternative approaches we should consider?



================
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-coroutines">;
 def CXXPre20CompatPedantic : DiagGroup<"pre-c++20-compat-pedantic",
----------------
aaron.ballman wrote:
> We don't typically give these their own warning group (it's also not clear why this would be a pedantic warning `warn_cxx17_compat_designated_init` is incorrect).
The reason for a separate warning group is to have a way to detect and prevent (via `-Werror`) the use of coroutines, but not other features.
`Wpre-c++20-compat-coroutines` might be a totally wrong name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247