aaron.ballman added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
- hasType,
- AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
- CXXBaseSpecifier),
+ hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
internal::Matcher<Decl>, InnerMatcher, 1) {
----------------
njames93 wrote:
> jkorous wrote:
> > aaron.ballman wrote:
> > > njames93 wrote:
> > > > aaron.ballman wrote:
> > > > > This is undoing a change that was just added less than two weeks ago,
> > > > > so I think the potential for breaking code is small. That said, can
> > > > > you explain why you think `hasClass` is a better approach than
> > > > > `hasType`?
> > > > Yeah, as that change hasn't reached landed onto a release branch
> > > > breaking code shouldn't be an issue, If it was I'd leave it in.
> > > >
> > > > - `hasType` is very generic, whereas `hasClass` is specific to what a
> > > > `CXXBaseSpecifier` supports.
> > > > - It makes the matchers marginally simpler.
> > > > `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"))))` vs
> > > > `hasDirectBase(hasClass(hasName("Base")))`
> > > > - In the documentation it also specifies that `hasClass` takes a
> > > > `Matcher<CXXRecordDecl>, making it more user friendly.
> > > FWIW, I prefer `hasType` to `hasClass`. You can inherit from things which
> > > are not a class, such as a struct (so the name is a bit of a misnomer,
> > > but not too awful), a class template (which you can't match with this
> > > interface), or a template type (which you also can't match with this
> > > interface).
> > I don't feel super strongly about this but I also slightly prefer `hasType`.
> >
> > To be fair - I didn't really have things like inheritance from template
> > parameters on my mind when working on `hasAnyBase` (it's definitely not
> > tested) so I'd rather not assume it works.
> I have decided to put `hasType` back in there as it does have some general
> uses. However I have added more class and class template specific matchers as
> I feel these are slightly more user friendly.
>
> LMK what you think of this approach.
>
> Side note what is the correct collective term for classes and structs. I'd be
> tempted to refer to them how clang does, records, but `hasRecord` seems wrong.
> Side note what is the correct collective term for classes and structs. I'd be
> tempted to refer to them how clang does, records, but hasRecord seems wrong.
We use the term "record", but I'm not certain how widely used that is.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher<CXXRecordDecl>,
+ InnerMatcher) {
----------------
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")))))`?
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
