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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.


================
Comment at: lib/AST/Expr.cpp:2562
+static Expr *IgnoreImpCastsSingleStep(Expr *E) {
+  if (auto *ICE = dyn_cast_or_null<ImplicitCastExpr>(E))
+    return ICE->getSubExpr();
----------------
aaron.ballman wrote:
> Do we really need to accept null arguments? We didn't previously, and this increases the overhead of something we do pretty often (though it's probably only a negligible performance hit).
I did benchmark it to see if there is any difference but could not measure any (looking at the generated code this adds a single null check at the start). My concern was that it could be possible that some AST node has a null child, perhaps when the AST is malformed. I will however defer to you or Richard since I probably lack some of the background needed to make this choice.


================
Comment at: lib/AST/Expr.cpp:2705
+template <typename FnTy1, typename FnTy2>
+Expr *IgnoreExprNodes(Expr *E, FnTy1 Fn1, FnTy2 Fn2) {
+  Expr *LastE = nullptr;
----------------
aaron.ballman wrote:
> I wonder if it would make more sense to use a parameter pack here so that this can be expanded to an arbitrary combination of single steps? The inner part of the while loop would look something like:
> ```
> template <typename ...Func>
> Expr *IgnoreExprNodes(Expr *E, Func&& ...Fns) {
>   ...
>   while (E != LastE) {
>     LastE = E;
>     for (auto &&Fn : {Fns...})
>       E = std::forward<decltype(Fn)>(Fn)(E);
>   }
>   ...
> }
> ```
I like it! Thanks for the suggestion. This particular solution did not work because it requires I believe all the types to be equal for the `auto` deduction to succeed. However a recursive solution works fine. I looked quickly at the generated code and it is identical as far as I can tell. Let me know if there are issues with it.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57267/new/

https://reviews.llvm.org/D57267