sammccall added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher<CXXRecordDecl>,
+ InnerMatcher) {
----------------
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.
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
