hwright marked 4 inline comments as done. hwright added inline comments.
================ Comment at: include/clang/ASTMatchers/ASTMatchers.h:3527 + return N < Node.getNumInits() && + InnerMatcher.matches(*Node.getInit(N)->IgnoreParenImpCasts(), Finder, + Builder); ---------------- klimek wrote: > aaron.ballman wrote: > > hwright wrote: > > > aaron.ballman wrote: > > > > I'm not certain we want the `IgnoreParenImpCasts()` here -- what if > > > > someone wants to match an initializer that uses one of those properties? > > > The `hasArg` implementation directly above has the same call to > > > `IgnoreParenImpCasts()`. Is it also in error? (It would seem that they > > > should both be consistent.) > > Yeah, I noticed that as well. Doing some code archaeology, it seems that > > `hasArg()` has had that form since its inception and it was never > > explicitly discussed what should happen there. > > > > @klimek -- do you have opinions here, since you wrote the original code for > > `hasArg()`? Should we be leaving the paren and implicit cast stripping up > > to the caller rather than doing it automatically? > Sigh, yea, this is one of those hard decisions: in the end, we decided that > the downside of surprise of users not seeing what they expected (due to an > imp cast) was less bad than basically taking away the ability to match imp > casts, so we stopped putting ignoreparenimpcasts everywhere. I'm not entirely > sure how to rate consistency with hasArg vs. getting us towards a more > explicit world, but my gut would probably go with leaving the > ignoreparenimpcasts out and see how bad people find it. Removed the call to `IgnoreParenImpCasts()`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56090/new/ https://reviews.llvm.org/D56090 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
