[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

I'm not sure if FileIndex is the correct layer to make decision about how References is calculated. Currently, we have two use cases in clangd 1) one slab per TU and 2) one slab per file. It seems to me that we would want different strategies for the two use cases, so it might make sense to make this configurable in FileIndex.  And in one case, we have `References` ~= # of TUs, and in the other case, we would have `References` ~= # of files. This can over-count `References`  in the second case. It might be fine as an approximation (if there is no better solution), but we should definitely document it (e.g. in `BackgroundIndex`).



================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:50
+        // FileIndex keeps only oneslab per file.
+        // This contradicts with behavior inside clangd-indexer, it only counts
+        // translation units since it processes each header multiple times.
----------------
this is a bit of a code smell. FileIndex is not supposed to know anything about clangd-indexer etc.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170
           I.first->second = mergeSymbol(I.first->second, Sym);
+        // We re-count number of references while merging refs from scratch.
+        I.first->getSecond().References = 0;
----------------
note that references might not always exist. SymbolCollector can be set up to not collect references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481