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

[Aaron Ballman via Phabricator via cfe-commits]

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


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:473
 
+TEST(MatcherCXXMemberCallExpr, InvokedAtType) {
+  auto M = cxxMemberCallExpr(invokedAtType(cxxRecordDecl(hasName("Y"))));
----------------
The test name is using the old name for the matcher.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:474
+TEST(MatcherCXXMemberCallExpr, InvokedAtType) {
+  auto M = cxxMemberCallExpr(invokedAtType(cxxRecordDecl(hasName("Y"))));
+  EXPECT_TRUE(matches(
----------------
Same here as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56851/new/

https://reviews.llvm.org/D56851