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