aaron.ballman added inline comments.
================ Comment at: lib/AST/Expr.cpp:2562 + return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null<FullExpr>(E)) ---------------- 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. 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
