riccibruno marked an inline comment as done. riccibruno added inline comments.
================ Comment at: lib/AST/Expr.cpp:2562 + return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null<FullExpr>(E)) ---------------- aaron.ballman wrote: > riccibruno wrote: > > It is something that is actually possible to audit. I did look at where > > each of the skipped node are created and it *seems* that a null child is > > not possible. However it is very easy to miss a case and adding the null > > check give the various `Expr::Ignore*` the following nice property: > > > > Regardless of the state of the AST, for every expression `E` (null or not). > > `Ignore*(E)` produces the correct result (which may be null). > > > > But perhaps the correct invariant to maintain is that these nodes have > > always a non-null child ? > > > > I re-ran the benchmarks more carefully and here are the results I got (on > > my usual benchmark: `-fyntax-only` on all of Boost, 20 iterations with > > `perf stat`): > > > > 8.2117 +- 0.0131 seconds (with the null check) > > 8.2028 +- 0.0058 seconds (without the null check) > > But perhaps the correct invariant to maintain is that these nodes have > > always a non-null child ? > > I believe we treat this as an invariant in a lot of other places, so it seems > like a reasonable invariant here. Being more resilient is not a bad thing, > but it may also mask broken invariants elsewhere. > > I'll leave it to @rsmith to make the final call though. All right. Lets go with `dyn_cast` to preserve the original behavior. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57267/new/ https://reviews.llvm.org/D57267 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
