This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] Add `cxxBaseSpecifier` matcher (non-top-level)
ClosedPublic

Authored by nick on Oct 19 2019, 1:59 PM.

Details

Summary

Required for capturing base specifier in matchers:

`cxxRecordDecl(hasDirectBase(cxxBaseSpecifier().bind("base")))`

Diff Detail

Event Timeline

nick created this revision.Oct 19 2019, 1:59 PM

Missing docs regeneration (clang/docs/tools/dump_ast_matchers.py i think)

Needs more tests - what happens with multiple inheritance, what about inheriting from inherited?

nick updated this revision to Diff 225770.Oct 19 2019, 3:15 PM

Regenerated the docs, added more tests.

nick updated this revision to Diff 225783.Oct 20 2019, 7:19 AM

Fixed typo.

steveire added a subscriber: steveire.

Could you rebase this? Parts of it were common with another patch which was merged.

nick updated this revision to Diff 331945.Mar 19 2021, 11:15 AM
nick retitled this revision from [clang][AST] Add `CXXBaseSpecifier` matcher support to [ASTMatchers] Add `cxxBaseSpecifier` matcher (non-top-level).
nick edited the summary of this revision. (Show Details)
nick added a reviewer: aaron.ballman.

Rebased. Removed things reimplemented and landed in D81552 + D79063.

nick updated this revision to Diff 331955.Mar 19 2021, 11:30 AM

Forgot to remove a duplicated test

aaron.ballman added inline comments.Mar 21 2021, 6:31 AM
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.

nick added inline comments.Mar 21 2021, 6:39 AM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
329

I do not understand why it should be here. The point of this test is to ensure that hasType can be used inside cxxBaseSpecifier. The support was added in D79063 without a test and I just fill the gaps.

aaron.ballman added inline comments.Mar 21 2021, 6:41 AM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
329

Oh shoot, good point, we already *have* those tests. :-)

nick updated this revision to Diff 332151.Mar 21 2021, 6:47 AM

Lint fixes

aaron.ballman accepted this revision.Mar 21 2021, 6:48 AM

LGTM, thank you!

This revision is now accepted and ready to land.Mar 21 2021, 6:48 AM
nick added a comment.Mar 21 2021, 12:57 PM

Could you please commit the patch for me? I do not have commit rights.

njames93 requested changes to this revision.Mar 21 2021, 4:38 PM
njames93 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
3847–3856

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.

This revision now requires changes to proceed.Mar 21 2021, 4:38 PM
nick added inline comments.Mar 21 2021, 6:18 PM
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
aaron.ballman added inline comments.Mar 22 2021, 4:57 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3847–3856

However overloading the matcher that takes a QualType matcher doesn't make sense as base specifiers have no qualifications.

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"))

Does anything prevent this going in now?

@nick Sorry that getting these changes merged takes so long.

@njames93 If you have an alternative way forward, please let us know what it is.

Otherwise, this LGTM too and we should merge it soon unless there are objections which haven't been addressed.

clang/unittests/ASTMatchers/Dynamic/RegistryTest.cpp
301

@nick Is this implemented in another MR? I don't see anything in your list of revisions. I think this is reasonable as is, but wondering if you intend to implement the top-level support too.

steveire accepted this revision.Mar 28 2021, 8:39 AM

@nick I think this also might need to be rebased again, sorry.

nick updated this revision to Diff 333728.Mar 28 2021, 11:31 AM

@nick Sorry that getting these changes merged takes so long.

@njames93 If you have an alternative way forward, please let us know what it is.

Otherwise, this LGTM too and we should merge it soon unless there are objections which haven't been addressed.

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.

nick added inline comments.Mar 28 2021, 11:34 AM
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.

@nick Sorry that getting these changes merged takes so long.

@njames93 If you have an alternative way forward, please let us know what it is.

Otherwise, this LGTM too and we should merge it soon unless there are objections which haven't been addressed.

I had a PR in Boost that took 4 years to merge, so it is nothing new to me :D

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

nick added a comment.Apr 3 2021, 12:52 PM

What name/email should I use for the commit?

Nikita Kniazev <kniazev.nikita -at- gmail.com>

This revision was not accepted when it landed; it landed in state Needs Review.Apr 8 2021, 4:06 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.