aaron.ballman added inline comments.
================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553 +/// \endcode +AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher<CXXRecordDecl>, + InnerMatcher) { ---------------- sammccall wrote: > aaron.ballman wrote: > > jkorous wrote: > > > aaron.ballman wrote: > > > > njames93 wrote: > > > > > jkorous wrote: > > > > > > aaron.ballman wrote: > > > > > > > jkorous wrote: > > > > > > > > Nit: while "[base specifier] `hasType`" sounds natural to me > > > > > > > > for some reason `hasClass` doesn't. English is not my first > > > > > > > > language though. > > > > > > > I agree that `hasClass` seems unnatural here. Out of curiosity, > > > > > > > could we modify the `hasName` matcher to work on base specifiers > > > > > > > so you can write: `cxxRecordDecl(hasAnyBase(hasName("Base")))` as > > > > > > > shorthand for the more wordy version > > > > > > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")))))`? > > > > > > Wouldn't it be strange to treat `hasName` differently than all the > > > > > > other narrowing matchers? Honest question - I feel that `hasName` > > > > > > might be the most commonly used, just don't know if that's enough > > > > > > to justify this. > > > > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers > > > > > Repurposing `hasName` would be a pain especially considering 99% of > > > > > its use cases wont be for base class matching. > > > > > Wouldn't it be strange to treat hasName differently than all the > > > > > other narrowing matchers? Honest question - I feel that hasName might > > > > > be the most commonly used, just don't know if that's enough to > > > > > justify this. > > > > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers > > > > > > > > Different how? I'm suggesting to overload `hasName` to work on a > > > > `CXXBaseSpecifier` since those have a name. > > > > > > > > > Repurposing hasName would be a pain especially considering 99% of its > > > > > use cases wont be for base class matching. > > > > > > > > I'm asking what the right API is for users, though, which is a bit > > > > different. Base specifiers have names (there are no unnamed base > > > > specifiers), so to me, it makes more sense for `hasName` to work with > > > > them directly since that is the thing that does name matching. > > > > > > > > I think you can accomplish this by using a > > > > `PolymorphicMatcherWithParam1` like we do for > > > > `hasOverloadedOperatorName` which can narrow to either a > > > > `CXXOperatorCallExpr` or a `FunctionDecl`. > > > >> Wouldn't it be strange to treat hasName differently than all the other > > > >> narrowing matchers? Honest question - I feel that hasName might be the > > > >> most commonly used, just don't know if that's enough to justify this. > > > >> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers > > > > > > > Different how? I'm suggesting to overload hasName to work on a > > > > CXXBaseSpecifier since those have a name. > > > > > > What I meant is that technically we can overload any > > > `Matcher<CXXRecordDecl>` matcher in the same fashion so having the > > > overloaded version of `hasName` only makes it somewhat special (and > > > someone might argue that it'd impact consistency of matchers > > > composability). Anyway, I'm fine with your suggestion! > > Ah, I see what you mean -- thanks for explaining. I'm on the fence about > > this. One the one hand, base specifiers *in the AST* do not have names, so > > it seems defensible to say that `hasName` should not handle a base > > specifier. On the other hand, base specifiers *in the language* are > > identifiers that always have a name, so it seems defensible to say that > > `hashName` should handle a base specifier. > > > > Pulling in some more folks to see if a wider audience brings consensus. > I think I agree that it's reasonable/consistent to do this, but I'm not sure > it's a good idea. > Base specifier -> type -> class -> name seems like the most *regular* > traversal that reflects the shape/concepts of the AST, shortcutting steps > makes the model (IMO) more complicated to make it more terse. > Is matching a base specifier with a hardcoded name really common enough in > absolute terms that people should keep this special case in their heads? > > I think `hasType(cxxRecordDecl(hasName("Base")))` a bit better, but i think > of *expressions* having types, and would prefer `specifiesType` or just > `specifies` at the outside. > But just my 2c and I'm not deeply familiar with the conventions here - I can > live with any of the options. > Is matching a base specifier with a hardcoded name really common enough in > absolute terms that people should keep this special case in their heads? Thanks for the perspective. I don't think base specifiers will be matched all that often, but I'm also not certain it's a special case because base specifiers are conceptually things with names even if they're not exposed as such via our AST. However, you bring up a good point about modelling the shape of the AST and that does suggest base specifier -> type -> class -> name as the right way to go. > I think hasType(cxxRecordDecl(hasName("Base"))) a bit better, but i think of > *expressions* having types, and would prefer specifiesType or just specifies > at the outside. If `hasType` only worked on expressions, I would be okay with adding `specifies` or `specifiesType` for this functionality, but since `hasType` works on expressions and declarations, I think it's more natural to reuse `hasType` for the specifier than to introduce `specifiesType` (or `hasClass`) for base specifiers (which name a class type). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81552/new/ https://reviews.llvm.org/D81552 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits