[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

[Bruno Ricci via Phabricator via cfe-commits]

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