[PATCH] D51605: [clangd] SymbolOccurrences -> Refs and cleanup

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for cleaning it up! I admit that `SymbolOccurrences` is a loong name. A few nits.



================
Comment at: clangd/index/FileIndex.cpp:120
+      auto &SymRefs = Sym.second;
+      std::sort(SymRefs.begin(), SymRefs.end());
+      std::copy(SymRefs.begin(), SymRefs.end(), back_inserter(RefsStorage));
----------------
So the sort is intended to make returned refs ordered?


================
Comment at: clangd/index/Index.h:367
+    return sizeof(*this) + Arena.getTotalMemory() +
+           sizeof(Refs.front()) * Refs.size();
   }
----------------
we should be careful if `Refs` is empty. Calling `front()` on empty container is undefined behavior. 


================
Comment at: clangd/index/MemIndex.h:23
+  /// Maps from a symbol ID to all corresponding Refs.
+  using RefMap = llvm::DenseMap<SymbolID, llvm::ArrayRef<Ref>>;
 
----------------
This type seems not used in this patch, consider removing it or using it to replace existing `llvm::DenseMap<SymbolID, llvm::ArrayRef<Ref>>` usage.


================
Comment at: unittests/clangd/FileIndexTests.cpp:341
+  std::vector<Ref> Results;
+  Index.refs(Request, [&Results](const Ref &O) { Results.push_back(O); });
 
----------------
My fault here. This is not safe, we need to store the underlying `FileURI` data.


================
Comment at: unittests/clangd/IndexTests.cpp:271
+  std::vector<Ref> Results;
+  MergedIndex->refs(Request, [&](const Ref &O) { Results.push_back(O); });
+
----------------
The same here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51605