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

Wed Dec 14 07:08:56 PST 2022

ilya-biryukov accepted this revision as: ilya-biryukov.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks a lot for driving this to conclusion! Please note that there is one small NIT you may want to fix before landing this.
UB definitely explains why we saw the bugs. It's good that we caught it before this landed.

In D129531#3992998 <https://reviews.llvm.org/D129531#3992998>, @ayzhao wrote:

> I don't think this should be an issue - `InitListExpr` doesn't take `ArrayFiller` into account in `computeDependence(...)` either: https://github.com/llvm/llvm-project/blob/c8647738cd654d9ecfdc047e480d05a997d3127b/clang/lib/AST/ComputeDependence.cpp#L636-L641

Yeah, makes sense, if `InitListExpr` does not take into account we are probably good here as well.
My intuition is that we can only figure out what to set in `ArrayFiller` when the expressions are non-dependent, therefore the filler itself should not add any extra "dependence" to the expression.
If that's the case, it would be nice to add an assertion. But that's unrelated to this change, we can do it some other day.

================
Comment at: clang/lib/Sema/SemaInit.cpp:6199
+          S.getLangOpts().CPlusPlus20 && RD && RD->isAggregate() && Failed() &&
+          Failure == FK_ConstructorOverloadFailed &&
+          onlyHasDefaultedCtors(getFailedCandidateSet())) {
----------------
NIT: use `getFailureKind()`, it asserts that `Failed() == true` and would have catched this particular UB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129531