sammccall added inline comments.
================ Comment at: clangd/index/Index.h:122 + + llvm::Optional<Details> Detail; + ---------------- 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. ================ Comment at: clangd/index/SymbolCollector.h:25 // changed. class SymbolCollector : public index::IndexDataConsumer { public: ---------------- 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. 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
