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

Fri Jul 28 08:39:12 PDT 2023

Mordante added a comment.

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

> In D156247#4540228 <https://reviews.llvm.org/D156247#4540228>, @Mordante wrote:
>
>> 2. My biggest concern is the timing of this patch. The date of the branching is know upfront and well communicated by the release managers. This patch was created after the LLVM-17 branch was created. For libc++ this patch would be a new feature which has no patch at the moment. This means we're asked to add a new feature after RC1. In libc++ we normally avoid adding new features in the release branch.
>
> I am sorry for sending the patch late, we have not thought about it before. I have already mentioned that before in the thread.

I've read that, before my reply. That does not change the fact I feel it is a concern.

> We could have foreseen the need for this change before, but we did not and there is nothing I can do to fix it now.

I understand.

>> - a pre-approval by the release managers to apply these changes to the release branch,
>
> I have asked the release managers above.

Thanks.

>> - a libc++ patch that works with `-fno-coroutines`, which I would be happy to review.
>
> I will check what needs to be done for libc++ here, I suspect adding `#error` inside `<coroutine>` if the feature macro is not defined should be enough.

This is not enough. There is quite a bit of work involved on the libc++ side. Here is a quick list of the things I directly can think of. First the patch in Clang needs to be available in our CI:

1. The patch needs to land in LLVM
2. We need to wait for the patch to be available in a build on apt.llvm.org (typically this takes one day, but may be longer if the build fails.)
3. We need to update and test our Docker image (this takes a few hours)
4. We need to upload the Docker image to the CI runners and wait for it to propagate (takes about 8 hours)

So it will take at least two days to get this working. The CI parts needs a libc++ developer who wants to do the work and have time in their schedule to work on this. When nobody has time, it will take longer to get the CI working.

Then the patch itself needs to do the following things (the work can be done directly, testing requires the CI)

1. Add `#error` inside `<coroutine>`
2. Add a LIBCPP feature flag and propagate that to lit
3. Adjust the tests that would fail with `-fno-coroutine`
4. Add a CI job so we can validate the new feature works

As @cor3ntin pointed out we need to be wary of regressions since other C++ features depend on coroutines. So we need a CI job to test whether the feature works correctly and do not regress when we add features depending on coroutines.

Maybe that helps a bit more to explain why this is a concern and not something we can do quickly on the libc++ side.

In D156247#4542155 <https://reviews.llvm.org/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 driver-level option. We should not be adding a novel language dialect as we're getting ready to cut a release unless the need and benefits are *very* compelling, and I don't think that's the case here. We need the time to think about the long-term effects of such a mode, so I don't think Clang 17 is an appropriate ship vehicle for this change, especially because Google can carry this patch in your downstream easily enough.
>
> My recommendation is to start an RFC on Discourse. If the community quickly and decisively agrees this should land in Clang 17 in the next few days, it might still be reasonable to make the rc2 cutoff, but I'd leave that decision to the release managers and it would be contingent on strong libc++ maintainer agreement to whatever direction we go (because they'll be the most impacted by this decision).

+1

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247