[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Aaron Ballman via Phabricator via cfe-commits]

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