[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This LG, but not accepting as I believe libc++ is still broken if built with modules, right?
I have run `check-cxx` locally and it seems to work, but I suspect that's not using modules by default. I have clicked through the links in phrabricator and the errors seem to come from the latest uploaded diff.
Let me know if I'm missing something.



================
Comment at: clang/lib/Sema/SemaInit.cpp:5380
+          }
+          InitExprs.push_back(ER.get());
+        }
----------------
ayzhao wrote:
> ilya-biryukov wrote:
> > ayzhao wrote:
> > > ayzhao wrote:
> > > > So the libc++ test compile failures are due to this line.
> > > > 
> > > > One example of a failing unit test is [range.take.while/ctor.view.pass](https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/ranges/range.adaptors/range.take.while/ctor.view.pass.cpp). Clang calls this function twice in `TreeTransform.h` - once with `VerifyOnly` set to `true`, once with it set to `false`.
> > > > 
> > > > For some reason, when this function tries to value-initialize the member `MoveOnly mo` in `View`, `Seq.Failed()` returns false after `TryValueInitialization(...)`, but the resulting `ExprResult` is `nullptr`, causing the segfault we see when we push `nullptr` to `InitExprs` and pass `InitExprs` to the constructor of `CXXParenListInitExpr`. One way to be fix this is to move the line `ExprResult ER = Seq.Perform(...)` out of the `if (!VerifyOnly)` block and check for `ER.isInvalid()` instead of `Seq.Failed()`, but that results in test failures due to excess diagnostic messages in `Seq.Perform(...)`
> > > > 
> > > > I'm still looking into this, but if anyone has any ideas, they would be very welcome.
> > > > 
> > > > To repro the buildbot failures, just build clang with this patch, and then in a separate build directory, build the target `check-cxx` using the previously built clang.
> > > I was able to get the above workaround to pass the test by clearing the diagnostics after calling `Seq.Perform(...)`.
> > > 
> > > IMO, this should be OK for now, but I'm open to better ideas if anyone has any.
> > Clearing all the diagnostics is a nuclear options and definitely seems off here. We should not call `Perform()` when `VerifyOnly` is `true` to avoid producing the diagnostics in the first place.
> > 
> > It's fine for the call with `VerifyOnly = true` to return no errors and later produce diagnostics with `VerifyOnly = false`, I believe this is what `InitListChecker` is already doing.
> > I have been playing around with the old version of the code, but couldn't fix it fully. I do have a small example that breaks, we should add it to the test and it should also be easier to understand what's going on:
> > 
> > ```
> > struct MoveOnly
> > {
> >   MoveOnly(int data = 1);
> >   MoveOnly(const MoveOnly&) = delete;
> >   MoveOnly(MoveOnly&&) = default;
> > };
> > 
> > struct View {
> >   int a;
> >   MoveOnly mo;
> > };
> > 
> > void test() {
> >   View{0};
> >   View(0); // should work, but crashes and produces invalid diagnostics.
> > }
> > ```
> > 
> > In general, my approach would be to try mimicing what `InitListChecker` is doing as much as possible, trimming all the unnecessary complexity that braced-init-lists entail.
> > Hope it's helpful.
> So it looks like all I had to do was remove the call to `TryValueInitialization(...)` and just check for `Seq.Failed()`. This is also what we do in `InitListChecker`.
> 
> The `check-cxx` target appears to work for me locally, so fingers crossed that the build passes.
> 
> >  I do have a small example that breaks, we should add it to the test and it should also be easier to understand what's going on:
> 
> Done.
Good catch! I have also noticed that `TryValueInitialization` is missing in `InitListChecker`, but somehow thought it's just doing the same thing without reusing the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531