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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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.

Could we find a compromise here? E.g.

- We would be fine with an "unsupported" configuration. We can fix any issues with it ourselves.
- Could we keep `-fno-coroutines` as an internal `-cc1` option and not advertise it?


Herald added a comment.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an `NFC` or refactoring, adding documentation etc..)

Add your unit tests in `clang/unittests/Format` and you can build with `ninja FormatTests`.  We recommend using the `verifyFormat(xxx)` format of unit tests rather than `EXPECT_EQ` as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in `TokenAnnotatorTest.cpp`



================
Comment at: clang/test/SemaCXX/coroutines.cpp:5
 // RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify=expected,cxx20_23,cxx23    %s -fcxx-exceptions -fexceptions -Wunused-result
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx14_20,cxx20_23 %s -fcxx-exceptions -fexceptions -Wunused-result
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify=expected,cxx14_20,cxx20,cxx20_23 %s -fcxx-exceptions -fexceptions -Wunused-result
+// RUN: %clang_cc1 -std=c++17 -fcoroutines -fsyntax-only -verify=expected,cxx14_20 %s -fcxx-exceptions -fexceptions -Wunused-result
----------------
Note to self: this is wrong and should be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247