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