ioeric added inline comments.
================ Comment at: clangd/index/Index.h:122 + + llvm::Optional<Details> Detail; + ---------------- 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. ================ Comment at: clangd/index/SymbolCollector.h:25 // changed. class SymbolCollector : public index::IndexDataConsumer { public: ---------------- sammccall wrote: > can you add a comment to the class indicating that it needs to be used for > one TU and then thrown away? This seems unfortunate but is probably simpler > than the alternative. It also seems to be a new restriction with this patch. How about re-initializing the allocator for each new AST in `intialize`? 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
