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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 22 12:04:05 PST 2017

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


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. 

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list