This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use ToolExecutor to write the global-symbol-builder tool.
ClosedPublic

Authored by ioeric on Jan 4 2018, 9:35 AM.

Event Timeline

ioeric created this revision.Jan 4 2018, 9:35 AM
hokein added a comment.Jan 5 2018, 1:51 AM

The implementation looks good, but to see whether sam has high-level comments on this.

clangd/index/SymbolYAML.cpp
140

The function could be simplified by using the SymbolToYAML below.

std::string Str;
for (const Symbol& S : Symbols) {
   Str += SymbolToYAML(S);
}
145

might be use Symbol as parameter type.

clangd/index/SymbolYAML.h
30

might be also mention The YAML result is safe to concatenate.

ioeric updated this revision to Diff 128723.Jan 5 2018, 2:54 AM
ioeric marked 2 inline comments as done.

Address review comments.

clangd/index/SymbolYAML.cpp
140

This saves two lines of code but would regress the performance, so I am inclined to the current approach.

This makes SymbolCollector (production code) depend on YAML (explicitly experimental), as well as on Tooling interfaces.
At least we should invert this by making the thing passed to SymbolCollector a function<void(const Symbol&)> or so.

Moreover, it's unfortunate we'd need to change SymbolCollector at all. Currently it's a nice abstraction that does one thing (AST -> SymbolSlab), now we're making it do two things. It can't be for scalability reasons - there's no problem fitting all the symbols from a TU in memory. So why do we need to do this?
I don't really understand the ToolExecutor interfaces well, but ISTM overriding SymbolIndexActionFactory::runInvocation to flush the symbols in the slab out to the ExecutionContext would allow you to use SymbolCollector unchanged?

ioeric updated this revision to Diff 129063.Jan 9 2018, 4:40 AM

Address review comment: move all ToolExecutor and YAML-specific logics out of SymbolCollector.

ioeric added a comment.Jan 9 2018, 4:46 AM

This makes SymbolCollector (production code) depend on YAML (explicitly experimental), as well as on Tooling interfaces.
At least we should invert this by making the thing passed to SymbolCollector a function<void(const Symbol&)> or so.

Thanks for the catch! I completely missed these perspectives...

Moreover, it's unfortunate we'd need to change SymbolCollector at all. Currently it's a nice abstraction that does one thing (AST -> SymbolSlab), now we're making it do two things. It can't be for scalability reasons - there's no problem fitting all the symbols from a TU in memory. So why do we need to do this?
I don't really understand the ToolExecutor interfaces well, but ISTM overriding SymbolIndexActionFactory::runInvocation to flush the symbols in the slab out to the ExecutionContext would allow you to use SymbolCollector unchanged?

WrapperFrontendAction is helpful here. All changes have been moved into the tool now. PTAL

sammccall accepted this revision.Jan 9 2018, 4:58 AM
This revision is now accepted and ready to land.Jan 9 2018, 4:58 AM
This revision was automatically updated to reflect the committed changes.