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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
