[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:105
+  /// What to insert when completing this symbol (plain text version).
+  std::string CompletionPlainInsertText;
+  /// What to insert when completing this symbol (snippet version).
----------------
sammccall wrote:
> insert texts can be in details I think? They're not required for completion until after resolve.
Quote from https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#completion-request
```
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.
```


================
Comment at: clangd/index/Index.h:122
+
+  llvm::Optional<Details> Detail;
+
----------------
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.


================
Comment at: clangd/index/SymbolCollector.cpp:65
+  CodeCompletionResult SymbolCompletion(ND, 0);
+  auto Allocator = std::make_shared<GlobalCodeCompletionAllocator>();
+  CodeCompletionTUInfo TUInfo(Allocator);
----------------
sammccall wrote:
> this doesn't seem like the kind of thing we should be allocating per-symbol!
> You can use a single one and Reset() it, I think
> 
> also why globalcodecompletionallocator, vs just codecompletionallocator?
> You can use a single one and Reset() it, I think
It's unclear how reset would affect `CodeCompletionTUInfo`, but we can simply maintain one allocator for each TU.

> also why globalcodecompletionallocator, vs just codecompletionallocator?
They are actually the same thing, but `CodeCompletionTUInfo` requires a global one. I don't really know why...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41345