Mordante added a comment.

In D156247#4538626 <https://reviews.llvm.org/D156247#4538626>, @aaron.ballman 
wrote:

> I would like explicit buy-in from the libc++ folks on the idea of adding 
> `-fno-coroutines` as they're going to be impacted by Clang supporting such an 
> option (and they may have additional testing requirements to ensure that 
> libc++ works reasonably well when coroutines are disabled).

Thanks for the adding libc++ to the review @aaron.ballman!

If some concerns with this patch

1. I share @philnik's concern that it adds a new configuration we need to test.
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.
3. I don't see a consensus from the Clang developers to add this option. I also 
see no consensus coroutines are "broken". I agree if we were to do something 
the `-fno-coroutines` would be the way to go.

I do not speak for Apple, but I know the beta's for the new XCode are available 
since June and I have not heard from @ldionne (who works at Apple) there were 
concerns with coroutines. @ldionne is out of the office at the moment. So even 
when we add this option now it will not be available in the next XCode. (Unless 
Apple backports it.)

@ilya-biryukov if you feel strongly about pursuing this patch I would like to 
see:

- a consensus of the Clang developers this is really needed,
- a pre-approval by the release managers to apply these changes to the release 
branch,
- a libc++ patch that works with `-fno-coroutines`, which I would be happy to 
review.

@ilya-biryukov the next time it would be really great to have such intrusive 
changes up for review well before the release branch is created. That gives the 
LLVM community time review this without the need to rush it. For me it would 
have been a lot easier to accept such a patch a month ago than it's now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156247

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to