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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 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) {
----------------
aaron.ballman wrote:
> 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.
https://en.cppreference.com/w/cpp/language/class - Going of what that says, it states that a class declaration starts with a keyword either `class` or `struct`. Nowhere on the page does it mention `record`.
Continuing on from this point, we have many more matchers with `class` in the name but work on structs too:
`ofClass`, `hasInClassInitializer` and `injectedClassNameType`. If you're being pedantic there is also `classTemplateDecl`, `classTemplatePartialSpecializationDecl` and `classTemplateSpecializationDecl`.

Having said all of that I'm still not a huge fan of `hasClass`, but I'm less of a fan of `hasType`. I'd thought of `forClass` but that could be misinterpreted as the derived class of the `CXXBaseSpecifier` Kind of like the behaviour of `forFunction`.
```
class Base {};
class Derived : Base {};
```
Does `cxxBaseSpecifier(forClass(cxxRecordDecl().bind("X")) bind to `Derived` or `Base`?


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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81552/new/

https://reviews.llvm.org/D81552