AFAIK this information isn't needed for retrieval/scoring, just for display.
LSP has completionItem/resolve that adds additional info to a completion item. This allows us to avoid sending a bunch of bulky comments, most of which will never be looked at.
In practice, there's nothing we particularly want to do differently for the memory index: we have to load the data into memory, and so including a pointer to it right away is no extra work.
However Symbol has two roles, and being the in-memory representation for MemIndex is only secondary. Its primary role is defining the protocol between indexes and clangd, including remote indexes where returning all documentation *is* expensive.
One option is to have Symbol just have the "core stuff" that's suitable for returning in bulk, and have an index method to retrieve more details that would be a point lookup only. (Maybe this is just the getSymbol method we've thought about). I'm not sure what it means for the data structure. OK if we discuss offline?
What are you planning to put here other than the return type of a function?
How are you planning to use this?
This seems to be related to the completion text/edits. We had some early discussions about whether we'd encode something like CodeCompletionString, or LSP snippets, or something else entirely.
it seems we'll want to share the(some?) doc logic between hover, AST-based complete, and indexing... See D35894 (which is ... complicated, no idea if it'll land soon).
Among other things:
Should these annotations go at the end?
How does this relate to the code in AST-based completion?
Thanks for the review!
Logic around CodeCompletionString is pulled into a separate file in D41450.
I also propagate the new completion info to completion code. I am happy to split it out if it adds too much noise for the review.
As discussed offline, putting non-core stuff in an optional structure.
As discussed offline, we now store the whole snippet in the insertion text.
I pulled CodeCompletionString handling logic into a separate file in D41450.
insert texts can be in details I think? They're not required for completion until after resolve.
I think you probably want a raw pointer rather than optional:
(raw pointer rather than unique_ptr because it's owned by a slab not by malloc, but unique_ptr is ok for now)
this doesn't seem like the kind of thing we should be allocating per-symbol!
also why globalcodecompletionallocator, vs just codecompletionallocator?
- Merged with origin/master
- Addressed some more comments.
- Add new fields to YAML.
The request can delay the computation od the detail and documentation properties. However, properties that are needed for the inital sorting and filtering, like sortText, filterText, insertText, and textEdit must be provided in the textDocument/completion request and must not be changed during resolve.
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.
llvm::Optional doesn't take much more space, so the size should be fine.
Could you elaborate on index-specific info? It's unclear to me how this would be used.
It's unclear how reset would affect CodeCompletionTUInfo, but we can simply maintain one allocator for each TU.
They are actually the same thing, but CodeCompletionTUInfo requires a global one. I don't really know why...
Oh no, somehow i missed this during review.
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.
This i'm less sure about, but I don't think it matters.
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?
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.
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.
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.
How about re-initializing the allocator for each new AST in intialize?
Just some nits
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.
you shouldn't initialize these both here and in initialize(), right?
why conditional? note we're interning the strings anyway, and an empty stringref is the same size as a full one
this removes the Detail object if it's empty - this seems maybe unneccesary and certainly the wrong layer. It seems enough to do
if (!outputting) Detail = (allocate) else if (!Detail) return; io.mapOptional("Documentation", Detail->Documentation); // etc