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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
