This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC.
ClosedPublic

Authored by ioeric on Sep 18 2018, 1:09 AM.

Details

Summary

FileIndex now provides explicit interfaces for preamble and main file updates.
This avoids growing parameter list when preamble and main symbols diverge
further (e.g. D52078). This also gets rid of the hack in indexAST that
inferred main file index based on TopLevelDecls.

Also separate indexMainDecls from indexAST.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Sep 18 2018, 1:09 AM
sammccall accepted this revision.Sep 18 2018, 2:06 AM

Glorious! Think the APIs could be simplified/clarified a little further, but up to you.

clangd/index/FileIndex.cpp
24 ↗(On Diff #165901)

assert DeclsToIndex is empty if !IsIndexMainAST?

clangd/index/FileIndex.h
77 ↗(On Diff #165901)

can't this just take PathRef + ParsedAST?
(same data, but clearer semantics)

114 ↗(On Diff #165901)

again, take ParsedAST here?

118 ↗(On Diff #165901)

This isn't quite right I think - we don't index the symbols that only occur in the main file.

120 ↗(On Diff #165901)

this shouldn't return any refs, just return SymbolSlab here

121 ↗(On Diff #165901)

indexPreamble would be clearer I think.

unittests/clangd/TestTU.cpp
51 ↗(On Diff #165901)

this should return FileIndex instead I think, with both preamble and main index.
But this requires fixing callers (since FileIndex isn't an index).
Leave a fixme?

(Maybe I should just bite the bullet and expose the MergedIndex class so we don't have to deal with these indirections...)

This revision is now accepted and ready to land.Sep 18 2018, 2:06 AM
ioeric updated this revision to Diff 165918.Sep 18 2018, 3:22 AM
ioeric marked 6 inline comments as done.
  • addressed review comments
clangd/index/FileIndex.h
121 ↗(On Diff #165901)

What about indexHeaderSymbols? indexPreamble seems to require understanding of how preamble is built, but it doesn't seem to matter here if AST was built from preamble or not.

sammccall added inline comments.Sep 18 2018, 3:27 AM
clangd/index/FileIndex.h
121 ↗(On Diff #165901)

indexHeaderSymbols is better, SGTM.

I kind of meant the "conceptual" preamble here - but headers is more accurate (mid-file #includes are possible), and has no risk of confusion with the concrete cacheable preamble.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
sammccall added inline comments.Sep 18 2018, 5:41 AM
clangd/index/FileIndex.h
77 ↗(On Diff #165901)

You're still taking the top-level decls, these can just come from the ParsedAST though?

ioeric added inline comments.Sep 18 2018, 6:37 AM
clangd/index/FileIndex.h
77 ↗(On Diff #165901)

Ah right. Sorry! Sent rL342473.