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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:46
+        auto It = MergedSyms->find(Sym.first);
+        assert(It != MergedSyms->end() && "Reference to unknown symbol!");
+        // Note that we increment References per each file mentioning the
----------------
ioeric wrote:
> I think we can trigger this assertion with an incomplete background index. For example, we've seen references of a symbol in some files but we've not parsed the file that contains the decls.
You are right, thanks for pointing this out!


================
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:
> kadircet wrote:
> > 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?
> the comment is useful. I just think it's in the wrong place. If we define the reference counting behavior for `FileSymbols`, `FileSymbols`wouldn't need to know about clangd-indexer or background-index. This comment can then live in background index instead.
Moved into index/background.h


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:189
           AllSymbols.push_back(&Sym);
+    mergeRefs(RefSlabs, RefsStorage, AllRefs);
     break;
----------------
ioeric wrote:
> any reason not to count references for `PickOne`?
Because PickOne is not populating SymsStorage and is merely passing along pointers to SymbolSlabs. Counting references in that case would imply also making a new copy per each unique symbol. 

Do you think it is worth it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481