Required for capturing base specifier in matchers:
`cxxRecordDecl(hasDirectBase(cxxBaseSpecifier().bind("base")))`Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Needs more tests - what happens with multiple inheritance, what about inheriting from inherited?
| clang/lib/ASTMatchers/Dynamic/Registry.cpp | ||
|---|---|---|
| 186 | Please keep this list sorted alphabetically. | |
| clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp | ||
| 329 | I'd like to see another test along these lines: struct Base {};
struct Intermediate : Base {};
struct Derived : Intermediate {};Where we test that Derived does not have a direct base relationship with Base, but does with hasAnyBase. | |
| clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp | ||
|---|---|---|
| 329 | Oh shoot, good point, we already *have* those tests. :-) | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 3847–3856 | I don't think the change to this matcher is warranted. | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 3847–3856 | What should I use in D69000 then? It is been a very long time since I developed and published these patches, but D69000 definitely requires this matcher, without it I get: llvm-project\clang\include\clang\ASTMatchers\ASTMatchersInternal.h(1569): error C2338: right polymorphic conversion llvm-project\clang-tools-extra\clang-tidy\modernize\DeprecatedIteratorBaseCheck.cpp(200): note: see reference to function template instantiation 'clang::ast_matchers::internal::PolymorphicMatcher<clang::ast_matchers::internal::matcher_hasType0Matcher,void (clang::ast_matchers::internal::TypeList<clang::Expr,clang::FriendDecl,clang::TypedefNameDecl,clang::ValueDecl>),clang::ast_matchers::internal::Matcher<clang::QualType>>::operator clang::ast_matchers::internal::Matcher<clang::CXXBaseSpecifier>(void) const<clang::CXXBaseSpecifier>' being compiled llvm-project\clang-tools-extra\clang-tidy\modernize\DeprecatedIteratorBaseCheck.cpp(192): note: see reference to function template instantiation 'clang::ast_matchers::internal::PolymorphicMatcher<clang::ast_matchers::internal::matcher_hasType0Matcher,void (clang::ast_matchers::internal::TypeList<clang::Expr,clang::FriendDecl,clang::TypedefNameDecl,clang::ValueDecl>),clang::ast_matchers::internal::Matcher<clang::QualType>>::operator clang::ast_matchers::internal::Matcher<clang::CXXBaseSpecifier>(void) const<clang::CXXBaseSpecifier>' being compiled | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 3847–3856 |
QualType is a convenience wrapper around a Type and so I think it's reasonable based on that convenience alone. We have some matchers that return a QualType, like asString(), which I could imagine a user wanting to use with cxxBaseSpecifier(). e.g., cxxBaseSpecifier(asString("SomeClass")) | |
Comment Actions
Comment Actions
I had a PR in Boost that took 4 years to merge, so it is nothing new to me :D
Rebased, even though there was no conflicts.
| clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp | ||
|---|---|---|
| 301 | The patch had some of the top-level matcher parts, but it cut them off when rebased. I have no need in it myself and I am not sure there is any kind of use for it. | |
Comment Actions
Yes, I'm a few years trying to get https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/ merged, but I've made some progress :D.
Rebased, even though there was no conflicts.
I think arc patch might be a bit fickle, so it wasn't applying cleanly for me before.
This LGTM and there have been no further objections, and the objection from @njames93 seems to be addressed.
What name/email should I use for the commit?
| clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp | ||
|---|---|---|
| 301 | If you look at the overloads of MatchFinder::addMatcher and compare to, say, the using declarations near the top of ASTMatchers.h or the types supported in ASTNodeKind or DynTypedNode::BaseConverter, this one is certainly "missing" as a supported top level node in MatchFinder::addMatcher (though it's not the only one in any case). That said I don't think merging this MR should depend on that feature. Hopefully someone will submit it for review. I have the same difficulty with getting changes reviewed, but I can review them and get them towards approval and merge more easily than I can find a reviewer :). | |
I don't think the change to this matcher is warranted.
The hasType matcher that accepts a DeclarationMatcher already has support for cxxBaseSpecifier.
However overloading the matcher that takes a QualType matcher doesn't make sense as base specifiers have no qualifications.