ymandel marked 2 inline comments as done.
ymandel 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>,
----------------
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)".
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
