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

[riccibruno]

Summary

Now that the implementation of all of the Expr::Ignore* is in Expr.cpp we can try to remove some duplication. Do this by separating the logic of the Expr::Ignore* from the iterative loop.

This is NFC, except for one change: IgnoreParenImpCasts now skips, among other things, everything that IgnoreImpCasts skips. This means FullExpr are now skipped by IgnoreParenImpCasts. This was likely an oversight when FullExpr was added to the nodes skipped by IgnoreImpCasts.

[riccibruno]

Update the comment in IgnoreImpCastsExtraSingleStep and return early from IgnoreImpCastsExtraSingleStep and IgnoreImplicitSingleStep when IgnoreImpCastsSingleStep skipped something.

[aaron.ballman]

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

lib/AST/Expr.cpp

2559–2567

I'd remove this comment.

2562

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).

2682–2692

I'd remove this comment as well.

2701

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);
  }
  ...
}

[riccibruno]

Removed the separator comments in Expr.cpp
Mode IgnoreExprNodes work with an arbitrary number of functions.

lib/AST/Expr.cpp

2562

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.

2701

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.

[aaron.ballman]

lib/AST/Expr.cpp

2562

If it doesn't seem to have a measurable performance impact, I think it's reasonable.

2563

No else after a return

2580

No else after a return (same elsewhere in the patch).

2668

Rather than an unnamed namespace, you should prefer static function declarations per the style guide.

2679

Formatting looks off here, did clang-format produce this?

[riccibruno]

Rebased and addressed Aaron's comments, except for the null check issue on which I am still undecided.

lib/AST/Expr.cpp

2562

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)

2679

It did, though with the static specifier clang-format adds a new line.

[aaron.ballman]

lib/AST/Expr.cpp

2562

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.

[riccibruno]

lib/AST/Expr.cpp

2562

All right. Lets go with dyn_cast to preserve the original behavior.

[riccibruno]

Go back to using dyn_casts.

[aaron.ballman]

LGTM! You should give @rsmith a few days in case he wants to express an opinion though.

lib/AST/Expr.cpp

2703–2705

Formatting (same elsewhere).

[riccibruno]

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

[void]

In D57267#1390484, @riccibruno wrote:

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

I don't think there was an explicit reason beyond "I didn't need to do it at the time". So probably just an oversight on my part. I don't know the code nearly as well as @rnk, so I could be wrong, but I think the existing tests should tell you if something went haywire if you skip FullExprs.

[rnk]

In D57267#1390484, @riccibruno wrote:

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

Yes, @rsmith asked me to skip all FullExprs, but that change did not pass tests, so I only made IgnoreParens ignore ConstantExpr to preserve existing behavior. There is no good design reason for it to be that way, and if you can adjust the callers to account for the new behavior, I think making them consistent would be good.

[riccibruno]

I don't think there was an explicit reason beyond "I didn't need to do it at the time". So probably just an oversight on my part. I don't know the code nearly as well as @rnk, so I could be wrong, but I think the existing tests should tell you if something went haywire if you skip FullExprs.

I see. I am a bit weary about relying on test coverage to validate a change. It is certainly a necessary condition but I am not sure that it is a sufficient condition.

Yes, @rsmith asked me to skip all FullExprs, but that change did not pass tests, so I only made IgnoreParens ignore ConstantExpr to preserve existing behavior. There is no good design reason for it to be that way, and if you can adjust the callers to account for the new behavior, I think making them consistent would be good.

I agree, but for now what I would like to do is just make sure that IgnoreParenImpCasts skips, among other things, every node skipped by IgnoreImpCasts. Not doing so is highly misleading given the names. (An other fun one is that you would think that IgnoreParenImpCasts() = IgnoreParens() + IgnoreImpCasts() but sadly that's not the case since IgnoreParenImpCasts() skips additionally MaterializeTemporaryExpr and SubstNonTypeTemplateParmExpr, even though IgnoreParenCasts() = IgnoreParens() + IgnoreCasts()).

[aaron.ballman]

In D57267#1391101, @riccibruno wrote:

I don't think there was an explicit reason beyond "I didn't need to do it at the time". So probably just an oversight on my part. I don't know the code nearly as well as @rnk, so I could be wrong, but I think the existing tests should tell you if something went haywire if you skip FullExprs.

I see. I am a bit weary about relying on test coverage to validate a change. It is certainly a necessary condition but I am not sure that it is a sufficient condition.

Yes, @rsmith asked me to skip all FullExprs, but that change did not pass tests, so I only made IgnoreParens ignore ConstantExpr to preserve existing behavior. There is no good design reason for it to be that way, and if you can adjust the callers to account for the new behavior, I think making them consistent would be good.

I agree, but for now what I would like to do is just make sure that IgnoreParenImpCasts skips, among other things, every node skipped by IgnoreImpCasts. Not doing so is highly misleading given the names. (An other fun one is that you would think that IgnoreParenImpCasts() = IgnoreParens() + IgnoreImpCasts() but sadly that's not the case since IgnoreParenImpCasts() skips additionally MaterializeTemporaryExpr and SubstNonTypeTemplateParmExpr, even though IgnoreParenCasts() = IgnoreParens() + IgnoreCasts()).

FWIW, I was rather disappointed in a recent review to learn that IgnoreParens() means "ignore parens... and a whole bunch of other stuff like generic selection expressions".

[riccibruno]

FWIW, I was rather disappointed in a recent review to learn that IgnoreParens() means "ignore parens... and a whole bunch of other stuff like generic selection expressions".

+1 Indeed.

include/clang/AST/Expr.h

public:
   }
 
   /// Skip past any parentheses and implicit casts which might surround this
   /// expression until reaching a fixed point.
   /// FIXME: IgnoreParenImpCasts really ought to be equivalent to
   /// IgnoreParens() + IgnoreImpCasts() until reaching a fixed point. However
   /// this is currently not the case. Instead IgnoreParenImpCasts() skips:
   /// * What IgnoreParens() skips.
-  /// * ImplicitCastExpr.
+  /// * What IgnoreImpCasts() skips.
   /// * MaterializeTemporaryExpr.
   /// * SubstNonTypeTemplateParmExpr.
   Expr *IgnoreParenImpCasts() LLVM_READONLY;
   const Expr *IgnoreParenImpCasts() const {
     return const_cast<Expr *>(this)->IgnoreParenImpCasts();
   }
 
   /// Skip past any parentheses and casts which might surround this expression

lib/AST/Expr.cpp

if (const BinaryOperator *op = dyn_cast<BinaryOperator>(expr)) {
     assert(type->isFunctionType());
     return type;
   }
 
   assert(isa<UnresolvedMemberExpr>(expr) || isa<CXXPseudoDestructorExpr>(expr));
   return QualType();
 }
 
-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* ****************************/
+
+static Expr *IgnoreImpCastsSingleStep(Expr *E) {
+  if (auto *ICE = dyn_cast_or_null<ImplicitCastExpr>(E))
+    return ICE->getSubExpr();
+
+  else if (auto *FE = dyn_cast_or_null<FullExpr>(E))
+    return FE->getSubExpr();
+
   return E;
 }
 
-Expr *Expr::IgnoreImplicit() {
-  Expr *E = this;
-  Expr *LastE = nullptr;
-  while (E != LastE) {
-    LastE = E;
-
-    if (auto *ICE = dyn_cast<ImplicitCastExpr>(E))
-      E = ICE->getSubExpr();
+static Expr *IgnoreImpCastsExtraSingleStep(Expr *E) {
+  // FIXME: Skip what IgnoreImpCastsSingleStep plus MaterializeTemporaryExpr
+  // and SubstNonTypeTemplateParmExpr to account for the current behaviour of
+  // IgnoreParenImpCasts().
+  E = IgnoreImpCastsSingleStep(E);
 
-    if (auto *FE = dyn_cast<FullExpr>(E))
-      E = FE->getSubExpr();
+  if (auto *MTE = dyn_cast_or_null<MaterializeTemporaryExpr>(E))
+    return MTE->GetTemporaryExpr();
 
-    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E))
-      E = MTE->GetTemporaryExpr();
+  else if (auto *NTTP = dyn_cast_or_null<SubstNonTypeTemplateParmExpr>(E))
+    return NTTP->getReplacement();
 
-    if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
-      E = BTE->getSubExpr();
-  }
   return E;
 }
 
-Expr* Expr::IgnoreParens() {
-  Expr* E = this;
-  while (true) {
-    if (ParenExpr* P = dyn_cast<ParenExpr>(E)) {
-      E = P->getSubExpr();
-      continue;
-    }
-    if (UnaryOperator* P = dyn_cast<UnaryOperator>(E)) {
-      if (P->getOpcode() == UO_Extension) {
-        E = P->getSubExpr();
-        continue;
-      }
-    }
-    if (GenericSelectionExpr* P = dyn_cast<GenericSelectionExpr>(E)) {
-      if (!P->isResultDependent()) {
-        E = P->getResultExpr();
-        continue;
-      }
-    }
-    if (ChooseExpr* P = dyn_cast<ChooseExpr>(E)) {
-      if (!P->isConditionDependent()) {
-        E = P->getChosenSubExpr();
-        continue;
-      }
-    }
-    if (ConstantExpr *CE = dyn_cast<ConstantExpr>(E)) {
-      E = CE->getSubExpr();
-      continue;
-    }
-    return E;
-  }
-}
+static Expr *IgnoreCastsSingleStep(Expr *E) {
+  if (auto *CE = dyn_cast_or_null<CastExpr>(E))
+    return CE->getSubExpr();
+
+  else if (auto *FE = dyn_cast_or_null<FullExpr>(E))
+    return FE->getSubExpr();
+
+  else if (auto *MTE = dyn_cast_or_null<MaterializeTemporaryExpr>(E))
+    return MTE->GetTemporaryExpr();
+
+  else if (auto *NTTP = dyn_cast_or_null<SubstNonTypeTemplateParmExpr>(E))
+    return NTTP->getReplacement();
+
 
-/// IgnoreParenCasts - Ignore parentheses and casts.  Strip off any ParenExpr
-/// or CastExprs or ImplicitCastExprs, returning their operand.
-Expr *Expr::IgnoreParenCasts() {
-  Expr *E = this;
-  while (true) {
-    E = E->IgnoreParens();
-    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;
-  }
+static Expr *IgnoreLValueCastsSingleStep(Expr *E) {
+  // Skip what IgnoreCastsSingleStep skips, except that only
+  // lvalue-to-rvalue casts are skipped.
+  if (auto *CE = dyn_cast_or_null<CastExpr>(E))
+    if (CE->getCastKind() != CK_LValueToRValue)
+      return E;
+
+  return IgnoreCastsSingleStep(E);
+
+static Expr *IgnoreBaseCastsSingleStep(Expr *E) {
+  if (auto *CE = dyn_cast_or_null<CastExpr>(E))
+    if (CE->getCastKind() == CK_DerivedToBase ||
+        CE->getCastKind() == CK_UncheckedDerivedToBase ||
+        CE->getCastKind() == CK_NoOp)
+      return CE->getSubExpr();
+
+  return E;
+
+static Expr *IgnoreImplicitSingleStep(Expr *E) {
+  E = IgnoreImpCastsSingleStep(E);
+
+  if (auto *MTE = dyn_cast_or_null<MaterializeTemporaryExpr>(E))
+    return MTE->GetTemporaryExpr();
+
+  else if (auto *BTE = dyn_cast_or_null<CXXBindTemporaryExpr>(E))
+    return BTE->getSubExpr();
+
+
+static Expr *IgnoreParensSingleStep(Expr *E) {
+  if (auto *PE = dyn_cast_or_null<ParenExpr>(E))
+    return PE->getSubExpr();
+
+  else if (auto *UO = dyn_cast_or_null<UnaryOperator>(E)) {
+    if (UO->getOpcode() == UO_Extension)
+      return UO->getSubExpr();
   }
 
-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;
-  }
-}
+  else if (auto *GSE = dyn_cast_or_null<GenericSelectionExpr>(E)) {
+    if (!GSE->isResultDependent())
+      return GSE->getResultExpr();
+
+  else if (auto *CE = dyn_cast_or_null<ChooseExpr>(E)) {
+    if (!CE->isConditionDependent())
+      return CE->getChosenSubExpr();
+
+  else if (auto *CE = dyn_cast_or_null<ConstantExpr>(E))
+    return CE->getSubExpr();
+
+  return E;
+
+static Expr *IgnoreNoopCastsSingleStep(const ASTContext &Ctx, Expr *E) {
+  if (auto *CE = dyn_cast_or_null<CastExpr>(E)) {
+    // We ignore integer <-> casts that are of the same width, ptr<->ptr and
+    // ptr<->int casts of the same width. We also ignore all identity casts.
+    Expr *SubExpr = CE->getSubExpr();
+    bool IsIdentityCast =
+        Ctx.hasSameUnqualifiedType(E->getType(), SubExpr->getType());
+    bool IsSameWidthCast =
+        (E->getType()->isPointerType() || E->getType()->isIntegralType(Ctx)) &&
+        (SubExpr->getType()->isPointerType() ||
+         SubExpr->getType()->isIntegralType(Ctx)) &&
+        (Ctx.getTypeSize(E->getType()) == Ctx.getTypeSize(SubExpr->getType()));
+
+    if (IsIdentityCast || IsSameWidthCast)
+      return SubExpr;
+
+  else if (auto *NTTP = dyn_cast_or_null<SubstNonTypeTemplateParmExpr>(E))
+    return NTTP->getReplacement();
+
 
-/// IgnoreParenLValueCasts - Ignore parentheses and lvalue-to-rvalue
-/// casts.  This is intended purely as a temporary workaround for code
-/// that hasn't yet been rewritten to do the right thing about those
-/// casts, and may disappear along with the last internal use.
-Expr *Expr::IgnoreParenLValueCasts() {
-  Expr *E = this;
-  while (true) {
-    E = E->IgnoreParens();
-    if (CastExpr *P = dyn_cast<CastExpr>(E)) {
-      if (P->getCastKind() == CK_LValueToRValue) {
-        E = P->getSubExpr();
-        continue;
-      }
-    } else if (MaterializeTemporaryExpr *Materialize
-                                      = dyn_cast<MaterializeTemporaryExpr>(E)) {
-      E = Materialize->GetTemporaryExpr();
-      continue;
-    } else if (SubstNonTypeTemplateParmExpr *NTTP
-                                  = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
-      E = NTTP->getReplacement();
-      continue;
-    } else if (FullExpr *FE = dyn_cast<FullExpr>(E)) {
-      E = FE->getSubExpr();
-      continue;
-    }
-    break;
+/************* Implementation of the various Expr::Ignore* ********************/
+
+namespace {
+/// Given an expression E and a function Fn : Expr * -> Expr *,
+/// Recursively apply Fn to E until reaching a fixed point.
+/// Note that a null E is valid; in this case nothing is done.
+template <typename FnTy> Expr *IgnoreExprNodes(Expr *E, FnTy Fn) {
+  Expr *LastE = nullptr;
+  while (E != LastE) {
+    LastE = E;
+    E = Fn(E);
+  return E;
+
+/// Given an expression E and functions Fn1, Fn2 : Expr * -> Expr *,
+/// Recursively apply Fn1 and Fn2 to E until reaching a fixed point.
+/// Note that a null E is valid; in this case nothing is done.
+template <typename FnTy1, typename FnTy2>
+Expr *IgnoreExprNodes(Expr *E, FnTy1 Fn1, FnTy2 Fn2) {
+  Expr *LastE = nullptr;
+  while (E != LastE) {
+    LastE = E;
+    E = Fn1(E);
+    E = Fn2(E);
   }
   return E;
 }
+} // anonymous namespace
 
-Expr *Expr::ignoreParenBaseCasts() {
-  Expr *E = this;
-  while (true) {
-    E = E->IgnoreParens();
-    if (CastExpr *CE = dyn_cast<CastExpr>(E)) {
-      if (CE->getCastKind() == CK_DerivedToBase ||
-          CE->getCastKind() == CK_UncheckedDerivedToBase ||
-          CE->getCastKind() == CK_NoOp) {
-        E = CE->getSubExpr();
-        continue;
-      }
-    }
+Expr *Expr::IgnoreImpCasts() {
+  return IgnoreExprNodes(this, IgnoreImpCastsSingleStep);
+
+Expr *Expr::IgnoreCasts() {
+  return IgnoreExprNodes(this, IgnoreCastsSingleStep);
 
-    return E;
-  }
+Expr *Expr::IgnoreImplicit() {
+  return IgnoreExprNodes(this, IgnoreImplicitSingleStep);
+
+Expr *Expr::IgnoreParens() {
+  return IgnoreExprNodes(this, IgnoreParensSingleStep);
 }
 
 Expr *Expr::IgnoreParenImpCasts() {
-  Expr *E = this;
-  while (true) {
-    E = E->IgnoreParens();
-    if (ImplicitCastExpr *P = dyn_cast<ImplicitCastExpr>(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;
-    }
-    return E;
-  }
+  return IgnoreExprNodes(this, IgnoreParensSingleStep,
+                         IgnoreImpCastsExtraSingleStep);
+
+Expr *Expr::IgnoreParenCasts() {
+  return IgnoreExprNodes(this, IgnoreParensSingleStep, IgnoreCastsSingleStep);
 }
 
 Expr *Expr::IgnoreConversionOperator() {
   if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(this)) {
     if (MCE->getMethodDecl() && isa<CXXConversionDecl>(MCE->getMethodDecl()))
       return MCE->getImplicitObjectArgument();
   }
   return this;
 }
 
-/// IgnoreParenNoopCasts - Ignore parentheses and casts that do not change the
-/// value (including ptr->int casts of the same size).  Strip off any
-/// ParenExpr or CastExprs, returning their operand.
-Expr *Expr::IgnoreParenNoopCasts(const ASTContext &Ctx) {
-  Expr *E = this;
-  while (true) {
-    E = E->IgnoreParens();
-
-    if (CastExpr *P = dyn_cast<CastExpr>(E)) {
-      // We ignore integer <-> casts that are of the same width, ptr<->ptr and
-      // ptr<->int casts of the same width.  We also ignore all identity casts.
-      Expr *SE = P->getSubExpr();
-
-      if (Ctx.hasSameUnqualifiedType(E->getType(), SE->getType())) {
-        E = SE;
-        continue;
-      }
-
-      if ((E->getType()->isPointerType() ||
-           E->getType()->isIntegralType(Ctx)) &&
-          (SE->getType()->isPointerType() ||
-           SE->getType()->isIntegralType(Ctx)) &&
-          Ctx.getTypeSize(E->getType()) == Ctx.getTypeSize(SE->getType())) {
-        E = SE;
-        continue;
-      }
-    }
+Expr *Expr::IgnoreParenLValueCasts() {
+  return IgnoreExprNodes(this, IgnoreParensSingleStep,
+                         IgnoreLValueCastsSingleStep);
 
-    if (SubstNonTypeTemplateParmExpr *NTTP
-                                  = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
-      E = NTTP->getReplacement();
-      continue;
-    }
+Expr *Expr::ignoreParenBaseCasts() {
+  return IgnoreExprNodes(this, IgnoreParensSingleStep,
+                         IgnoreBaseCastsSingleStep);
 
-    return E;
-  }
+Expr *Expr::IgnoreParenNoopCasts(const ASTContext &Ctx) {
+  return IgnoreExprNodes(this, IgnoreParensSingleStep, [&Ctx](Expr *E) {
+    return IgnoreNoopCastsSingleStep(Ctx, E);
+  });
 }
 
 bool Expr::isDefaultArgument() const {
   const Expr *E = this;
   if (const MaterializeTemporaryExpr *M = dyn_cast<MaterializeTemporaryExpr>(E))
     E = M->GetTemporaryExpr();
 
   while (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E))