This is an archive of the discontinued LLVM Phabricator instance.

[AST] Add TypeLoc support to node introspection
ClosedPublic

Authored by steveire on Apr 14 2021, 6:02 PM.

Details

Summary

[AST] Add TypeLoc support to node introspection

Extend the matchers gathering API for types to record template
parameters. The TypeLoc type hierarchy has some types which are
templates used in CRTP such as PointerLikeTypeLoc. Record the inherited
template and template arguments of types inheriting those CRTP types in
the ClassInheritance map. Because the name inherited from is now
computed, the value type in that map changes from StringRef to
std::string. This also causes the toJSON override signature used to
serialize that map to change.

Remove the logic for skipping over empty ClassData instances. Several
classes such as TypeOfExprTypeLoc inherit a CRTP class which provides
interesting locations though the derived class does not. Record it as a
class to make the locations it inherits available.

Record the typeSourceInfo accessors too as they provide access to
TypeLocs in many classes.

The existing unit tests use UnorderedElementsAre to compare the
introspection result with the expected result. Our current
implementation of google mock (in gmock-generated-matchers.h) is limited
to support for comparing a container of 10 elements. As we are now
returning more than 10 results for one of the introspection tests,
change it to instead compare against an ordered vector of pairs.

Because a macro is used to generate API strings and API calls, disable
clang-format in blocks of expected results. Otherwise clang-format
would insert whitespaces which would then be compared against the
introspected strings and fail the test.

Introduce a recursion guard in the generated code. The TypeLoc class
has IgnoreParens() API which by default returns itself, so it would
otherwise recurse infinitely.

Diff Detail

Event Timeline

steveire created this revision.Apr 14 2021, 6:02 PM
steveire requested review of this revision.Apr 14 2021, 6:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 6:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire edited the summary of this revision. (Show Details)Apr 15 2021, 4:52 AM
njames93 added inline comments.Apr 15 2021, 2:39 PM
clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp
152–153

nit: can't these be moved into the if stmt below, while were here StmtOrDeclBase can also be moved inside the if.

168–172

Can we have a comment explaining why we are discarding these?

clang/unittests/Introspection/IntrospectionTest.cpp
889

This test seems irrelevant to what this patch is trying to achieve.

1257

Can you add a comment explaining the issues with this test on windows platforms.

steveire updated this revision to Diff 337923.Apr 15 2021, 3:53 PM
steveire edited the summary of this revision. (Show Details)

Update

clang/unittests/Introspection/IntrospectionTest.cpp
1257

I don't know what the problem is, but it failed: https://reviews.llvm.org/B98791

njames93 added inline comments.Apr 15 2021, 11:08 PM
clang/unittests/Introspection/IntrospectionTest.cpp
1257

I'm guessing on windows the typeof extension is disabled, I'd guess you could try buildASTFromCodeWithArgs and pass -std=gnu++11. However in all honesty I'd be happy if you just added a comment to say this test doesn't work on windows due to use of the typeof extension.

njames93 accepted this revision.Apr 17 2021, 3:51 AM

LGTM, just a few nits before landing please.

clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp
25–42

nit: Is it not easier to just override getCheckTraversalKind for the class?

168–172

nit: can this be addressed?

clang/unittests/Introspection/IntrospectionTest.cpp
200–201

nit: Don't create a vector here unnecessarily, same below.

This revision is now accepted and ready to land.Apr 17 2021, 3:51 AM
This revision was automatically updated to reflect the committed changes.
steveire added inline comments.Apr 17 2021, 3:28 PM
clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp
168–172

There is a comment above, so I'm not sure what's missing. But please feel free to adjust it post commit!