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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
