alexfh 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:
> 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?
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
