[libcxx-commits] [clang] [libcxx] [llvm] "Reapply "[Sema] Fix crash on invalid code with parenthesized aggrega… (PR #76833)

[Mark de Wever via libcxx-commits]

mordante wrote:

> > It's not ideal, but I can't think of a better solution. Typically nightly apt builds are built nightly so then we can update the Docker CI quickly. If the nightly builds are red it might take a bit longer.
> > The patch is in draft since I wanted to test with the bootstrap build which required temporary disabling other builds (the second commit). When the CI is green I'll do a final review and publish the patch.
> 
> It's been 2 days, maybe we should just submit this and see if anything goes wrong? Besides, the CI seems green now, so hopefully everything has already been checked.

I'll land this soon. Yesterday evening I didn't have time.

> > > In the long-term, how should people land changes like this? Compiler diagnostics are expected to change sometimes, so we should figure out some way that allows landing these without reverts. Any thoughts?
> > 
> > 
> > Typically we work together with whom changes the tests. When there are libc++ changes it is visible in our review queue. For changes in diagnostics it's often possible to use a regex matcher. For example, the messages of `static_asserts` changed a while back. In general we prefer to minimize the tests depending on the compiler diagnostics.
> 
> This use-case is different, we need a way to change the set of reported diagnostics in Clang even if the diagnostic is produced in libc++ tests. Blocking changes to Clang because libc++ tests against nightly and head builds looks too harsh.
> 
> I think we need to find some alternative solution that works here. The obvious solution that I see is to run the nightly Clang as post-submits and allow them to be broken for this reasons (as it will fix itself after nightly Clang gets rebuilt).

All but one of the libc++ pre-commit CI jobs uses pre-built clang binaries. We heavily depend on a pre-commit CI to test changes on platforms we don't have access to. So changing it to a post-commit CI is not an option.

> @AaronBallman, @cor3ntin what are your thoughts as Clang maintainers? TLDR; is that libc++ runs tests in CI with both head Clang and nightly Clang builds. Therefore, if we happen to change the set of reported diagnostics in Clang (in this case it was a spurious error in initialization of invalid classes that got silenced with a Clang change), we cannot land this change without breaking the libc++ CI.

Note that the original patch also breaks clang-16 and clang-17. So usually there's a regex matcher used and that works with clang-16, clang-17, clang-18 nightly, and clang-18 ToT. This is a rare case where it's not possible to use a regex. (In general clang patches affecting libc++ are rare.) As mentioned before typically we work together with Clang to see how to fix this. As far as I can tell the changes to libc++ were not reviewed by the a libc++ reviewer and not tested in the pre-commit CI. Please correct me if I'm wrong.

After a previous breakage both @ldionne and I have discussed this with @AaronBallman and that resulted in [documentation](https://clang.llvm.org/hacking.html#testingLibc++) to create awareness and we helped to get a clang pre-commit CI up and running.

https://github.com/llvm/llvm-project/pull/76833