sammccall added inline comments.
================ Comment at: clangd/index/FileIndex.cpp:167 + } + // FIXME: aggregate symbol reference count based on references. + for (const auto &Sym : FileSyms.getValue()->Refs) { ---------------- This is a good FIXME to carry over to FileSymbols::buildIndex(). It may (or may not) make sense to control this behavior with the same param that controls merge behavior, the logic being that it's also a quality/accuracy tradeoff. ================ Comment at: clangd/index/FileIndex.h:49 +/// rename the existing FileSymbols to something else e.g. TUSymbols? +class SymbolsGroupedByFiles { +public: ---------------- `FileSymbols` isn't actually that opinionated about the data it manages: - the keys ("filenames") just identify shards so we can tell whether a new shard is an addition or replaces an existing one. - the values (tu symbols/refs) are merged using pretty generic logic, we don't look at the details. I think we should be able to use `FileSymbols` mostly unmodified, and store digests in parallel, probably in `BackgroundIndexer`. Is there a strong reason to store "main file" digests separately to header digests? There are a couple of weak points: - The merging makes subtle assumptions: for symbols it picks one copy arbitrarily, assuming there are many copies (merging would be expensive) and they're mostly interchangeable duplicates. We could add a constructor or `buildIndex()` param to FileSymbols to control this, similar to the IndexType param. (For refs it assumes no duplicates and simply concatenates, which is also fine for our purposes). - `FileSymbols` locks internally to be threadsafe while keeping critical sections small. To keep digests in sync with FileSymbols contents we probably have to lock externally with a second mutex. This is a little ugly but not a real problem I think. ================ Comment at: clangd/index/IndexAction.h:31 + std::function<void(RefSlab)> RefsCallback, + std::function<void(FileDigests)> FileDigestsCallback); ---------------- these parallel callbacks that always get called together are a bit ridiculous (my fault). Can you add a FIXME to replace them with a single callback that passes a struct? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53433 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits