[PATCH] D41345: [clangd] Add more symbol information for code completion.

[Eric Liu via Phabricator via cfe-commits]

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