[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:97
+/// paths used by \p FileSyms.
+void shardSlabToFiles(const SymbolSlab &Syms, const RelationSlab &Rels,
+                      FileSymbols &FileSyms, llvm::StringRef HintPath) {
----------------
sammccall wrote:
> This screams out to be shared with BackgroundIndex::update(). Is it very hard to do?
> 
> Things that are different:
>  - no duplication of symbol into defining file (why?)
>  - refs not gathered here (but I guess empty input -> empty output?)
>  - include graph not gathered here (same)
>  - this holds 3 copies of all the data at peak, other impl holds 2
> 
> I imagine factoring out a function that returns a StringMap<File> like in BackgroundIndex::Update, but File has a build() function that returns an IndexFileIn or so.
> (Ooh, is depending on IndexFileIn a layering violation? Maybe we should move that into another header. Duplicating the struct is probably OK for now)
In backgroundindex we "prefilter" symbols and only duplicate the ones coming from modified files, whereas in this one we duplicate every symbol no matter what. I didn't want to change backgroundindex into post filtering mode (I suppose that's what you mean by the last item).

as for the first bullet point, duplicating in case of preamble symbols wouldn't matter since we don't merge(rather pick one) while building the index for preamblesymbols. I suppose storing them twice shouldn't be a huge issue.

Depending to `IndexFileIn` in "Index.h" would be a violation, but depending on it in "FileIndex.h" is fine.

I am happy to refactor a `StringMap<File> shardIndexToFiles(IndexFileIn, HintPath)` into "FileIndex.h" that can be used by both backgroundindex and this, if you are OK with above mentioned regressions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77732/new/

https://reviews.llvm.org/D77732