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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 2 inline comments as done.
kadircet added a comment.

In D59481#1432881 <https://reviews.llvm.org/D59481#1432881>, @ioeric wrote:

> 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.


`References` ~= # of TUs case has nothing to do with FileIndex actually, it only happens if references are counted beforehand(which is currently only happening in clangd-indexer).
FileIndex has two ways to handle duplicates during an index merge:

- `pickone` which doesn't count references at all (this is the oneslab per TU case)
- `merge` which counts references as number of files this symbol has occured (this is the oneslab per File case)

> 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`).

I don't follow why this can over-count, FileIndex keeps only one RefSlab per each file, so we can't over-count? Did you mean it will be more than #TUs ?



================
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.
----------------
ioeric wrote:
> this is a bit of a code smell. FileIndex is not supposed to know anything about clangd-indexer etc.
I've actually just left the comment for review so that we can decide what to do about discrepancy(with my reasoning). It is not like FileIndex is somehow tied to clangd-indexer now?


================
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;
----------------
ioeric wrote:
> note that references might not always exist. SymbolCollector can be set up to not collect references.
I thought we wouldn't make any promises about `References` in such a case, do we?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481