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.

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

assert DeclsToIndex is empty if !IsIndexMainAST?

clangd/index/FileIndex.h
77

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

114

again, take ParsedAST here?

118

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

120

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

121

indexPreamble would be clearer I think.

unittests/clangd/TestTU.cpp
51

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

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

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

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

Ah right. Sorry! Sent rL342473.