[PATCH] D56090: Add a matcher for members of an initializer list expression

[Hyrum Wright via Phabricator via cfe-commits]

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