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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

Adding Richard for opinions on whether `IgnoreParenImpCasts` should skip full expressions, and also the changes to accepting null expressions as arguments in more places.



================
Comment at: lib/AST/Expr.cpp:2559
 
-Expr *Expr::IgnoreImpCasts() {
-  Expr *E = this;
-  while (true)
-    if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E))
-      E = ICE->getSubExpr();
-    else if (FullExpr *FE = dyn_cast<FullExpr>(E))
-      E = FE->getSubExpr();
-    else
-      break;
+/************* Logic for the various Expr::Ignore* ****************************/
+
----------------
I'd remove this comment.


================
Comment at: lib/AST/Expr.cpp:2562
+static Expr *IgnoreImpCastsSingleStep(Expr *E) {
+  if (auto *ICE = dyn_cast_or_null<ImplicitCastExpr>(E))
+    return ICE->getSubExpr();
----------------
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).


================
Comment at: lib/AST/Expr.cpp:2686
 
-Expr *Expr::IgnoreCasts() {
-  Expr *E = this;
-  while (true) {
-    if (CastExpr *P = dyn_cast<CastExpr>(E)) {
-      E = P->getSubExpr();
-      continue;
-    }
-    if (MaterializeTemporaryExpr *Materialize
-        = dyn_cast<MaterializeTemporaryExpr>(E)) {
-      E = Materialize->GetTemporaryExpr();
-      continue;
-    }
-    if (SubstNonTypeTemplateParmExpr *NTTP
-        = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
-      E = NTTP->getReplacement();
-      continue;
-    }
-    if (FullExpr *FE = dyn_cast<FullExpr>(E)) {
-      E = FE->getSubExpr();
-      continue;
-    }
-    return E;
+/************* Implementation of the various Expr::Ignore* ********************/
+
----------------
I'd remove this comment as well.


================
Comment at: lib/AST/Expr.cpp:2705
+template <typename FnTy1, typename FnTy2>
+Expr *IgnoreExprNodes(Expr *E, FnTy1 Fn1, FnTy2 Fn2) {
+  Expr *LastE = nullptr;
----------------
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);
  }
  ...
}
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D57267