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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added a comment.

Thanks!



================
Comment at: clangd/index/Index.cpp:39
+                             [](const Symbol &S, const SymbolID &I) {
+                               return S.ID == I;
+                             });
----------------
ilya-biryukov wrote:
> Should this be `S.ID < I`?
Wow, good catch (both here and below)!
Added a test (for some reason I thought we had one for this sort of stuff)


================
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.
----------------
hokein wrote:
>  use `Symbols.shrink_to_fit()`?
This is only a hint, and on my system it's a no-op. I think we know enough about the lifecycle here to make forcing the shrink worth it.


================
Comment at: clangd/index/SymbolCollector.h:36
   // All Symbols collected from the AST.
-  SymbolSlab Symbols;
+  SymbolSlab::Builder Symbols;
 };
----------------
hokein wrote:
> Maybe name it `SymbolBuilder` to avoid confusion -- `Symbols` indicates its type is `SymbolSlab`. 
Hmm, I'm not sure `SymbolBuilder` is a better name - it doesn't build symbols.
I'm not really seeing the association between `Symbols` and `SymbolSlab` - a `SymbolSlab::Builder` is also a collection of symbols. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41506