[PATCH] D56851: [ASTMatchers] Adds `CXXMemberCallExpr` matcher `invokedAtType`.

[Alexander Kornienko via Phabricator via cfe-commits]

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