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

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@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?

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 {};



================
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:
> > 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.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher<CXXRecordDecl>,
+              InnerMatcher) {
----------------
Nit: while "[base specifier] `hasType`" sounds natural to me for some reason `hasClass` doesn't. English is not my first language though.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552