[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 8 12:57:02 PST 2019
aaron.ballman added a comment.
In D57267#1391101 <https://reviews.llvm.org/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".
CHANGES SINCE LAST ACTION
More information about the cfe-commits