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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D57267