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