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

[Ilya Biryukov via Phabricator via cfe-commits]

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 `={}` for it.

Sorry if this was discussed before and I'm repeating myself, I vaguely remember something similar but not sure what the conclusion was.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:116
+      RefSlabs.push_back(FileAndRefs.second.first);
+      if (FileAndRefs.second.second)
+        MainFileRefs.push_back(RefSlabs.back().get());
----------------
Let's use a named struct here? `.second.second` is super-hard to read


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:125
+    // Keeps index to symbol in SymsStorage.
+    llvm::DenseMap<SymbolID, size_t> Merged;
     for (const auto &Slab : SymbolSlabs) {
----------------
Could we avoid changing the merge logic?
I.e. the proposal is to keep the merging code the same and add a loop that counts references at the end.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.h:60
+///
+/// Will count Symbol::References based on number of references in the main
+/// files, while building the index with DuplicateHandling::Merge option.
----------------
NIT: this comment seems more appropriate at `buildIndex`, maybe move it there?
(It has the `DuplicateHandle` parameter, so it's closer to the context.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.h:66
   /// If either is nullptr, corresponding data for \p Path will be removed.
+  /// If IsMainFile is true, Refs will be used for counting References during
+  /// merging.
----------------
Maybe call the parameter `CountReferences`?
It would narrow down what the code can do with it and make the code easier to understand


================
Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:49
+MATCHER_P(NumReferences, N, "") {
+  return arg.References == static_cast<unsigned>(N);
+}
----------------
NIT: to avoid warnings, we could use the corresponding suffix at the callsite (e.g. `1u`) instead of casts
It's probably ok either way here, but having casts can lead to surprises if one passes negative numbers there


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481