aaron.ballman added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3300
+/// matches `x.m()` and `p->m()`.
+AST_MATCHER_P_OVERLOAD(clang::CXXMemberCallExpr, invokedAtType,
+ clang::ast_matchers::internal::Matcher<clang::QualType>,
----------------
ymandel wrote:
> alexfh wrote:
> > ymandel wrote:
> > > aaron.ballman wrote:
> > > > ymandel wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > ymandel wrote:
> > > > > > > > ymandel wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > alexfh wrote:
> > > > > > > > > > > The name of the matcher doesn't tell me much. I had to
> > > > > > > > > > > carefully read the documentation to understand what is it
> > > > > > > > > > > about. I don't have a name that would raise no questions
> > > > > > > > > > > and wouldn't be too verbose at the same time, but a bit
> > > > > > > > > > > of verbosity wouldn't hurt I guess. How about
> > > > > > > > > > > `objectTypeAsWritten`?
> > > > > > > > > > Yeah, I think this would be a better name. Also, having
> > > > > > > > > > some examples that demonstrate where this behavior differs
> > > > > > > > > > from `thisPointerType` would be helpful.
> > > > > > > > > Agreed that it needs a new name, but I'm having trouble
> > > > > > > > > finding one I'm satisfied with. Here's the full description:
> > > > > > > > > "the type of the written implicit object argument". I base
> > > > > > > > > this phrasing on the class CXXMemberCallExpr's terminology.
> > > > > > > > > In `x.f(5)`, `x` is the implicit object argument, whether or
> > > > > > > > > not it is also implicitly surrounded by a cast. That is,
> > > > > > > > > "implicit" has two different meanings in this context.
> > > > > > > > >
> > > > > > > > > So, with that, how about `writtenObjectType`? It's close to
> > > > > > > > > `objectTypeAsWritten` but I'm hoping it makes more clear that
> > > > > > > > > the "written" part is the object not the type.
> > > > > > > > I've contrasted the behavior with thisPointerType in both of
> > > > > > > > the examples. Do you think this helps or do you want something
> > > > > > > > more explicit?
> > > > > > > Here's a totally different direction: `onOrPointsToType()`
> > > > > > > ```
> > > > > > > cxxMemberCallExpr(onOrPointsToType(hasDeclaration(cxxRecordDecl(hasName("Y")))))
> > > > > > > ```
> > > > > > >
> > > > > > I think more explicit would be better. e.g.,
> > > > > > ```
> > > > > > cxxMemberCallExpr(invokedAtType(hasDeclaration(cxxRecordDecl(hasName("X")))))
> > > > > > matches 'x.m()' and 'p->m()'.
> > > > > > cxxMemberCallExpr(on(thisPointerType(hasDeclaration(cxxRecordDecl(hasName("X"))))))
> > > > > > matches nothing because the type of 'this' is 'Y' in both cases.
> > > > > > ```
> > > > > But, what about even simpler: onType? I think this parallels the
> > > > > intuition of the name thisPointerType. onType(T) should match x.f
> > > > > and x->f, where x is type T.
> > > > You've pointed out why I don't think `onType` works -- it doesn't match
> > > > on type T -- it matches on type T, or a pointer/reference to type T,
> > > > which is pretty different. Someone reading the matcher may expect an
> > > > exact type match and insert a `pointerType()` or something there
> > > > thinking they need to do that to match a call through a pointer.
> > > >
> > > > @alexfh, opinions?
> > > True. I should have explained more.
> > >
> > > 1. Ultimately, I think that none of these names really make sense on
> > > their own and the user will need some familiarity with the documentation.
> > > I spent quite a while trying to come up with better names and didn't find
> > > anything compelling. I think that `onType` benefits from not carrying
> > > much information -- reducing the likelihood of misunderstanding it
> > > (they'll have to read the documentation) while paralleling the meaning of
> > > the matcher `on` and the behavior of `thisPointerType` (which also allows
> > > either the type or the pointer to that type).
> > >
> > > 2. My particular concern with `onOrPointsToType` is that it sounds like
> > > the "or" applies to the `on` but it really means "on (type or points to
> > > type)".
> > So far, my observations are:
> > 1. three engineers quite familiar with the topic can't come up with a name
> > that would explain the concept behind this matcher
> > 2. anyone reading that name would have to look up the documentation
> > 3. the implementation of the matcher is straightforward and even shorter
> > than the documentation
> >
> > Should we give up and let users just type `on(anyOf(hasType(Q),
> > hasType(pointsTo(Q))))`?
> >
> > If we want a bit more brevity here, maybe introduce a
> > `hasTypeOrPointsToType` matcher (any bikeshed color will do ;) to shorten
> > the expression above?
> Yes to both suggestions (dropping this one and adding
> `hasTypeOrPointsToType`). It seems a rather obvious conclusion now that
> you've said it. :)
>
> Personally, I'd go with `hasOrPointsToType`, but agreed that its just bike
> shedding. Aaron?
>
> I'll drop this diff and create a new one for the new matcher.
> Personally, I'd go with hasOrPointsToType, but agreed that its just bike
> shedding. Aaron?
I think `hasOrPointsToType` is sufficiently clear within this context, but it
makes me wonder if users are going to then need
`hasOrPointsToOrReferencesType()` for other situations?
I am kind of leaning towards just letting users spell the matcher out long-hand
as `on(anyOf(hasType(Q), hasType(pointsTo(Q))))`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56851/new/
https://reviews.llvm.org/D56851
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
