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

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE360344: [clangd] Count number of references while merging RefSlabs inside FileIndex (authored by kadircet, committed by ). Changed prior to commit:

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

kadircet updated this revision to Diff 198816. kadircet marked 2 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59481/new/ https://reviews.llvm.org/D59481 Files:

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:101 FileToRefs.erase(Path); - else -FileToRefs[Path] = std::move(Refs); + else { +

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

kadircet updated this revision to Diff 198770. kadircet marked 7 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59481/new/ https://reviews.llvm.org/D59481 Files:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/Background.cpp:482 FileDigest Digest; +bool IsMainFile; }; NIT: maybe initialize with `=false` to avoid potential UB. `Digest` seems uninitialized too, could also `={}`

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

kadircet updated this revision to Diff 197288. kadircet added a comment. - Change logic to count references from only main files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59481/new/ https://reviews.llvm.org/D59481 Files:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/Background.h:113 + // Note that FileSymbols counts References by incrementing it per each file + // mentioning the symbol, including headers. This contradicts with the kadircet

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

kadircet marked 2 inline comments as done. kadircet added a comment. It has been a long time since I've proposed that change and even I forgot some of the high level details. Therefore, I wanted to sum up the state again so that we can decide on how to move forward. Currently we have two

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/Background.h:113 + // Note that FileSymbols counts References by incrementing it per each file + // mentioning the symbol, including headers. This contradicts with the We should

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

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

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

kadircet updated this revision to Diff 191844. kadircet marked 11 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59481/new/ https://reviews.llvm.org/D59481 Files:

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

ilya-biryukov added inline comments. 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. +

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

ioeric added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:33 +// If MergedSyms is provided, increments a symbols Reference count once for +// every file it was mentioned in. +void mergeRefs(llvm::ArrayRef> RefSlabs, nit: s/every

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

kadircet marked an inline comment as done. kadircet added inline comments. 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. +

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

ioeric added a comment. > 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 ? Yes. This is how `Symbol::References` is described in its documentation. If we want to change/expand the

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

kadircet marked 2 inline comments as done. kadircet added a comment. In 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)

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

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

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

kadircet created this revision. kadircet added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, jdoerfert, arphaman, mgrang, jkorous, MaskRay, ioeric. Herald added a project: clang. For counting number of references clangd was relying on merging every duplication of a symbol.