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

Wed Jul 26 03:27:10 PDT 2023

cor3ntin added a comment.

In D156247#4534645 <https://reviews.llvm.org/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 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?

Designated initializers have this warning because they have been backported to older language mode, and they have
a different warning in these modes https://gcc.godbolt.org/z/9acefPW1n (which is important for pedantic conformance), and i guess the c++20 warning helps identify portability issues when trying newer standards on code bases that use these extensions.

But that does not apply to coroutines that are not available in older language modes.

Which for me brings a few questions. Why is it an issue for coroutines specifically? Concepts are also rough on the edges (but we do not set up the feature test macro), and consteval has (or had until very recently)
some bugs too (including code gen bugs, ironically!)

So the only warning that would make sense to me is "Support for coroutines is still experimental." I think that's what you want to express here.
Which again begs the question of why we claim conformance for something we think is too unstable to use in production. 
Three of the four bugs above are miss compilation of code that does not look particularly unusual, so I'm not sure the number of users matters. Whoever encounters that code will have an unpleasant time. 
And as you say. few people can work their way around such bugs

But if we think coroutines are good enough for one groups of users but not another... i'm not sure the warning belongs upstream.
Because the logical conclusion of that is that we would have to add warnings for any features added to the language.... which i don't think would benefit the ecosystem.

Having a separate warning group (ie `pre-c++20-compat-coroutines`) also makes me wonder about future clang versions:

- Either we remove the warning group and break build scripts
- We keep the warning group forever
- We keep the group but stop emitting the warning, which is also not great.

Sadly, i do not have the expertise to look at codegen bugs but maybe we should prioritize them higher in the hope of fixing some of them in the next few weeks.
If we could identify the problematic patterns, i think emitting an error from codegen until we fix them would also be reasonable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247