Page MenuHomePhabricator

Make the FindTypes(std::vector<CompilerContext>, ...) API testable in lldb-test
ClosedPublic

Authored by aprantl on Aug 19 2019, 4:34 PM.

Details

Summary

I'm planning to modify this API shortly and thought best to first make it independently testable.

This adds a -compiler-context=<...> option to lldb-test that translates a JSON-formatted string that is a list of kind/name pairs and translates it into a std::vector<CompilerContext>, a CompilerContext being a pair of context-kind and name.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Aug 19 2019, 4:34 PM
JDevlieghere added inline comments.Aug 19 2019, 5:25 PM
lldb/tools/lldb-test/lldb-test.cpp
145 ↗(On Diff #216020)

...as a JSON array...`

This makes me wonder though, wouldn't it be better to pass a file here and have it live in inputs? Do you expect it to be different across tests or could we reuse the context across tests?

aprantl marked an inline comment as done.Aug 19 2019, 5:27 PM
aprantl added inline comments.
lldb/tools/lldb-test/lldb-test.cpp
145 ↗(On Diff #216020)

I don't really expect a DeclContexts to have more than 3 entries in the forseeable future, so that would arguable just make the test harder to read.

JDevlieghere added inline comments.Aug 19 2019, 5:45 PM
lldb/tools/lldb-test/lldb-test.cpp
145 ↗(On Diff #216020)

But would those entries be the same across tests or not? If they're the same I think it'd be better to reuse them, even if it's only a few entries.

I am glad you're finding lldb-test useful. Please see my inline comment about quoting.

lldb/lit/SymbolFile/DWARF/compilercontext.ll
1 ↗(On Diff #216020)

We generally put test data into an Inputs subfolder. Though in this case, it looks like you could actually inline the data into the test file below. (Feel free to add .ll into the list of test suffixes.)

lldb/tools/lldb-test/lldb-test.cpp
145 ↗(On Diff #216020)

I actually think it is nice to have the entries present in the test file directly. However, what I think will be an issue is that you need to use two layers of quotes to pass a json string over the command line. I expect this will cause problems on windows, because quoting works differently there.

What I would actually suggest is to ditch JSON, and use a custom format for passing this. Maybe something like --compiler-context Module:CModule,Module:SubModule,.... That would be both shorter and completely avoid quotes. I don't think is should make parsing this signtificantly more difficult than what you have now.

aprantl updated this revision to Diff 216174.Aug 20 2019, 9:13 AM

Address Pavel's comments.

aprantl marked an inline comment as done.Aug 20 2019, 9:14 AM
aprantl added inline comments.
lldb/tools/lldb-test/lldb-test.cpp
145 ↗(On Diff #216020)

But would those entries be the same across tests or not? If they're the same I think it'd be better to reuse them, even if it's only a few entries.

No, they would be highly specific to the test. A context is a search criterium, something like "any class type called Foo nested inside a module called A".

labath accepted this revision.Aug 20 2019, 9:23 AM

lgtm

lldb/tools/lldb-test/lldb-test.cpp
33 ↗(On Diff #216174)

I guess this is not needed now.

This revision is now accepted and ready to land.Aug 20 2019, 9:23 AM
JDevlieghere added inline comments.Aug 20 2019, 9:23 AM
lldb/tools/lldb-test/lldb-test.cpp
145 ↗(On Diff #216020)

Okay, in that case passing them on the command line is definitely the way to go.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 9:47 AM