ioeric added inline comments.
================ Comment at: clangd/index/Index.h:122 + + llvm::Optional<Details> Detail; + ---------------- sammccall wrote: > 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. Done. Made it a raw pointer. PTAL 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
