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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added a comment.

In D81552#2086420 <https://reviews.llvm.org/D81552#2086420>, @jkorous wrote:

> @njames93 `hasDirectBase` seems like a useful matcher to me! OTOH I am not totally convinced about `hasType` -> `hasClass`. Anyway, don't you want to land `hasDirectBase` as a separate patch first and then discuss the rest?


There more I add to this, the more splitting it up sounds like a good idea.

> One more thing - I'm just thinking if there isn't some corner case where a base class could be interpreted as both direct and indirect. The most ugly case I came up with is virtual inheritance. I admit I don't know what the standard says about this so it might be a non-issue. Also, it'd still probably make sense to match on such base. WDYT?
> 
>   struct Base {};
>   struct Proxy : virtual Base {};
>   struct Derived : Base, Proxy {};

In that case it would probably register as a direct and indirect base. However there is no matcher(nor much need for one) that will solely match indirect bases so its mostly a non issue.



================
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) {
----------------
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.


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:3158
+    class Base {};
+    class Derived : BAse {};
+  )cc",
----------------
jkorous wrote:
> Is this (`Base` != `BAse`) a typo or a way how to tease the asserts?
It was to tease the assers, but I removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552