ymandel marked 3 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: > > 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))))` Sure. I'll drop this revision and we can revisit in the future if we see sufficient demand. 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