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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits