aaron.ballman added reviewers: sammccall, dblaikie. aaron.ballman added subscribers: sammccall, dblaikie. aaron.ballman added a comment.
Pinging @klimek , @sammccall , and @dblaikie to see if there are some opinions about overloading `hasName` (and possibly other naming related questions). ================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553 +/// \endcode +AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher<CXXRecordDecl>, + InnerMatcher) { ---------------- 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. 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