This is an archive of the discontinued LLVM Phabricator instance.

Introduce clang-query tool.
ClosedPublic

Authored by pcc on Nov 3 2013, 10:35 PM.

Details

Summary

This tool is for interactive exploration of the Clang AST using AST matchers.
It currently allows the user to enter a matcher at an interactive prompt
and view the resulting bindings as diagnostics, AST pretty prints or AST
dumps. Example session:

$ cat foo.c
void foo(void) {}
$ clang-query foo.c --
clang-query> match functionDecl()

Match #1:

foo.c:1:1: note: "root" binds here
void foo(void) {}
^~~~~~~~~~~~~~~~~
1 match.

Diff Detail

Event Timeline

klimek added inline comments.Nov 4 2013, 12:46 AM
clang-query/QueryEngine.cpp
34 ↗(On Diff #5337)

This seems like a strong indication that we want virtual dispatch here :) (I thought that when reading the header, but this kinda drives it home)

clang-query/QueryEngine.h
28 ↗(On Diff #5337)

I'm a very strong proponent of "public-first". The reason is that a reader of the class usually starts at the top, and a user of a class should not care about the privates, and thus be able to skip them - this is considerably easier if they're at the bottom of the class definition.

37 ↗(On Diff #5337)

Some doxygen comments would be highly appreciated ;)

clang-query/tool/ClangQuery.cpp
50–57 ↗(On Diff #5337)

Can we add a -c "..." or -f <command-file> batch mode? This would also allow us to add a FileCheck integration test...

sbenza added inline comments.Nov 4 2013, 8:53 AM
clang-query/QueryParser.cpp
22 ↗(On Diff #5337)

isWhitespace() from clang/Basic/CharInfo.h

clang-query/QueryParser.h
18 ↗(On Diff #5337)

StringRef instead of const char*

pcc updated this revision to Unknown Object (????).Nov 5 2013, 11:18 PM
  • Address reviewer comments
clang-query/QueryEngine.cpp
34 ↗(On Diff #5337)

I wanted to keep Query a pure data structure in order to simplify the parser tests. But of course there are other considerations. Done.

clang-query/QueryEngine.h
28 ↗(On Diff #5337)

I believe this comment is moot following the design change.

37 ↗(On Diff #5337)

Added.

clang-query/QueryParser.cpp
22 ↗(On Diff #5337)

Done.

clang-query/QueryParser.h
18 ↗(On Diff #5337)

I'd prefer not to. The implementation of the parser is simpler when the string is known to be null terminated.

clang-query/tool/ClangQuery.cpp
50–57 ↗(On Diff #5337)

I think both would be useful; done.

klimek accepted this revision.Nov 6 2013, 9:15 AM

lg with the nits fixed

clang-query/Query.cpp
55–65 ↗(On Diff #5371)

Yea, we really need addDynamicMatcher ...

clang-query/Query.h
46 ↗(On Diff #5371)

I kinda dislike the introduction of RTTI just for testing ... Since this is all in clang-query I think it's ok, as I cannot come up with a significantly better way right now.

clang-query/QueryParser.h
22 ↗(On Diff #5371)

Here also StringRef...

sbenza added inline comments.Nov 6 2013, 9:29 AM
clang-query/Query.cpp
55–65 ↗(On Diff #5371)

If I remember correctly, I added one, then we decide to remove it again.
This tool would be the first use case that would require it.

58 ↗(On Diff #5371)

You can bind at the DynTypedMatcher level using DynTypedMatcher::tryBind(). No need to do it for every type.

59 ↗(On Diff #5371)

What is the purpose of forEachDescendant() here?
Finder will recursively try to match the provided matchers already.

Doing it this way will match the topmost node if something in it matches, instead of returning the actual node that matched.

clang-query/QueryParser.h
20 ↗(On Diff #5371)

There should be a comment somewhere specifying the valid commands and their syntax.
It could in the parser, or somewhere in ClangQuery.cpp (possibly printing it to the user with a --help option).

silvas added inline comments.Nov 6 2013, 12:51 PM
clang-query/Query.h
60–61 ↗(On Diff #5371)

These return true classof's should be here. See http://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html. In particular, see the second rule of thumb http://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html#rules-of-thumb.

pcc updated this revision to Unknown Object (????).Nov 6 2013, 4:35 PM
  • Address reviewer comments
pcc added inline comments.Nov 6 2013, 4:36 PM
clang-query/Query.cpp
55–65 ↗(On Diff #5371)

I've resurrected it in D2114.

58 ↗(On Diff #5371)

Done.

59 ↗(On Diff #5371)

MatchFinder::match will not match recursively, so we need to do it ourselves with forEachDescendant.

However, upon further reflection, I've decided that it would be better to have a MatchFinder::matchAST that does the same thing for ASTs as the ASTConsumer returned by MatchFinder::getConsumer. See D2115.

clang-query/Query.h
60–61 ↗(On Diff #5371)

I assume you meant "should not be here". Done.

clang-query/QueryParser.h
20 ↗(On Diff #5371)

Done.

22 ↗(On Diff #5371)

OK, done.

pcc closed this revision.Nov 7 2013, 4:12 PM

Closed by commit rL194227 (authored by @pcc).

kwk added a subscriber: kwk.Sep 2 2014, 3:22 AM
kwk added a comment.Sep 24 2014, 7:01 AM

Can you please explain a usage of the "set bind-root" command? Why would I not want to match the loaded AST against a given matcher?

clang-tools-extra/trunk/clang-query/Query.cpp
38

What does this mean? And why would I not want to match the loaded AST against a given matcher? Can you please give a useful small example of how to use the "set bind-root" command?

klimek added inline comments.Sep 24 2014, 8:15 AM
clang-tools-extra/trunk/clang-query/Query.cpp
38

I think you're misreading this; the doc for bind-root is below ("Set whether to bind the root matcher to "root".)