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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
