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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156247#4538664 <https://reviews.llvm.org/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++ folks to come back with feedback, what would be the desired behavior. GCC seems to
>
> 1. ~~errors out on `#include <coroutine>`~~ (undefines feature test macros, so STL shows an error),
> 2. treats coroutine keywords as identifiers.
>
> While (1) makes total sense, I'm not sure about (2). Should we instead show errors, but still treat `co_await` and friends as keywords?
> I think it would be a better choice because it would prevent writing non-standard-compliant code.
> On the other hand, given the `co_` prefix, the clashes should be **really** rare anyway and we might want to match GCC behavior.
>
> Thoughts?

If we allow the feature to be disabled, it should be fully disabled. I doubt people are going to need to disable the feature because they have `co_yield` as an identifier in their program, but just the same, it'd be pretty strange to have a feature only partially disabled.

In D156247#4539048 <https://reviews.llvm.org/D156247#4539048>, @ilya-biryukov wrote:

> In D156247#4538962 <https://reviews.llvm.org/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 configurations we support and adding style-guide configurations won't help that. IMO this should just be a clang-tidy check. They can be enforced just as well as compiler warnings/errors, and don't cause problems for other people.
>
> I don't want to go too much into detail, but unfortunately we cannot enforce this with a tidy check.
> We do not have reliable ways to mark all shared code that is compiled by multiple toolchains (e.g. Apple Clang and stable Clang). Consequently we cannot guarantee that tidy runs on that code.
> Moreover, linters like clang-tidy can be accidentally disabled or ignored and we want strict enforcement for this particular change.

I'm not certain this is particularly motivating rationale for why Clang should allow people to turn off arbitrary standards features.

> Could we find a compromise here? E.g.
>
> - We would be fine with an "unsupported" configuration. We can fix any issues with it ourselves.

Anything we do that's user facing is something we have to support. I don't think it makes sense to add a language dialect mode and tell users "if you use this, we won't support it."

> - Could we keep `-fno-coroutines` as an internal `-cc1` option and not advertise it?

That would be an approach to ensuring this is not a supported configuration and that any fallout from using the flag is on the user rather than the community.

One interesting thing to note is that GCC doesn't seem to advertise `-fno-coroutines` as a supported option (https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/C_002b_002b-Dialect-Options.html#index-fcoroutines). They document other `-ffoo`/`-fno-foo` options, so the lack of documentation for `-fno-coroutines` could perhaps be indicative that this isn't intended to be a supported command line flag for GCC. We should reach out to some GCC folks to see if this is an oversight in their documentation or not. If there's not a GCC compatibility story here, that may change opinions on how/if to proceed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247