[PATCH] D41506: [clangd] Use Builder for symbol slabs, and use sorted-vector for storage

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/Index.cpp:76
+SymbolSlab SymbolSlab::Builder::build() && {
+  Symbols = {Symbols.begin(), Symbols.end()}; // Force shrink-to-fit.
+  // Sort symbols so the slab can binary search over them.
----------------
 use `Symbols.shrink_to_fit()`?


================
Comment at: clangd/index/Index.cpp:81
+  // We may have unused strings from overwritten symbols. Build a new arena.
+  BumpPtrAllocator Arena;
+  DenseSet<StringRef> Strings;
----------------
Maybe rename it to `NewArena`, we already have a member called `Arena`.


================
Comment at: clangd/index/Index.h:174
+     std::vector<Symbol> Symbols;
+     llvm::DenseMap<SymbolID, size_t> SymbolIndex;
+  };
----------------
worth a comment saying the value of the map is the vector index of `Symbols`, it took me a while to understand.


================
Comment at: clangd/index/Index.h:182
+  llvm::BumpPtrAllocator Arena;
+  std::vector<Symbol> Symbols;
 };
----------------
Mention the Symbols is required to be sorted.


================
Comment at: clangd/index/SymbolCollector.h:36
   // All Symbols collected from the AST.
-  SymbolSlab Symbols;
+  SymbolSlab::Builder Symbols;
 };
----------------
Maybe name it `SymbolBuilder` to avoid confusion -- `Symbols` indicates its type is `SymbolSlab`. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41506