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

Reply via email to