sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Just some nits ================ Comment at: clangd/index/Index.h:122 + + llvm::Optional<Details> Detail; + ---------------- ioeric wrote: > sammccall wrote: > > ioeric wrote: > > > sammccall wrote: > > > > I think you probably want a raw pointer rather than optional: > > > > - reduce the size of the struct when it's absent > > > > - make it inheritance-friendly so we can hang index-specific info off > > > > it > > > > (raw pointer rather than unique_ptr because it's owned by a slab not by > > > > malloc, but unique_ptr is ok for now) > > > > > > > This is not easy for now with `unique_ptr` because of this line :( > > > https://github.com/llvm-mirror/clang-tools-extra/blob/3565d1a1a692fc9f5c21e634b470535da2bb4d25/clangd/index/SymbolYAML.cpp#L141). > > > > > > > > > This shouldn't be an issue when we have the optimized symbol slab, where > > > we store raw pointers. And we would probably want to serialize the whole > > > slab instead of the individual symbols anyway. > > > > > > > reduce the size of the struct when it's absent > > > `llvm::Optional` doesn't take much more space, so the size should be fine. > > > > > > > make it inheritance-friendly so we can hang index-specific info off it > > > Could you elaborate on `index-specific info`? It's unclear to me how this > > > would be used. > > > This is not easy for now with unique_ptr because of this line > > Oh no, somehow i missed this during review. > > We shouldn't be relying on symbols being copyable. I'll try to work out how > > to fix this and delete the copy constructor. > > > > > This shouldn't be an issue when we have the optimized symbol slab, where > > > we store raw pointers. > > Sure. That's not a big monolithic/mysterous thing though, storing the > > details in the slab can be done in this patch... If you think it'll be > > easier once strings are arena-based, then maybe we should delay this patch > > until that's done, rather than make that work bigger. > > > > > And we would probably want to serialize the whole slab instead of the > > > individual symbols anyway. > > This i'm less sure about, but I don't think it matters. > > > > > llvm::Optional doesn't take much more space, so the size should be fine. > > Optional takes the same size as the details itself (plus one bool). This is > > fairly small for now, but I think a major point of Details is to expand it > > in the future? > > > > > Could you elaborate on index-specific info? It's unclear to me how this > > > would be used. > > Yeah, this is something we talked about in the meeting with Marc-Andre but > > it's not really obvious - what's the point of allowing Details to be > > extended if clangd has to consume it? > > > > It sounded like he might have use cases for using index infrastructure > > outside clangd. We might also have google-internal index features we want > > (matching generated code to proto fields?). I'm not really sure how > > compelling this argument is. > Thanks for the allocator change! > > `Details` now contains just stringrefs. I wonder how much we want it to be > inherit-friendly at this point, as the size is relatively small now. If you > think this is a better way to go, I'll make the structure contain strings and > store the whole structure in arena. This would require some tweaks for yamls > tho but shouldn't be hard. So this needs to be at least optional I think - at the moment, how does an API user know whether to fetch the rest of the details? FWIW, the size difference is already significant: symbol is 136 bytes if this is a unique_ptr, and 164 bytes if it's optional (+20%) - StringRefs are still pretty big. But I don't feel really strongly about this. ================ Comment at: clangd/index/SymbolCollector.cpp:71 +SymbolCollector::SymbolCollector() + : CompletionAllocator(std::make_shared<GlobalCodeCompletionAllocator>()), + CompletionTUInfo(CompletionAllocator) {} ---------------- you shouldn't initialize these both here and in `initialize()`, right? ================ Comment at: clangd/index/SymbolCollector.cpp:140 + S.CompletionPlainInsertText = PlainInsertText; + if (PlainInsertText != SnippetInsertText) + S.CompletionSnippetInsertText = SnippetInsertText; ---------------- why conditional? note we're interning the strings anyway, and an empty stringref is the same size as a full one Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41345 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
