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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the review!

Logic around CodeCompletionString is pulled into a separate file in https://reviews.llvm.org/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.



================
Comment at: clangd/index/Index.h:92
 
+  // Documentation including comment for the symbol declaration.
+  std::string Documentation;
----------------
sammccall wrote:
> 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?
As discussed offline, putting non-core stuff in an optional structure.


================
Comment at: clangd/index/Index.h:100
+  std::string CompletionDetail;
+  // The placeholder text for function parameters in order.
+  std::vector<std::string> Params;
----------------
sammccall wrote:
> 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.
> Honestly it would be great to have a doc describing this mapping between source -> index -> LSP for completion data.
As discussed offline, we now store the whole snippet in the insertion text.


================
Comment at: clangd/index/SymbolCollector.cpp:61
+
+std::string getDocumentation(const CodeCompletionString &CCS) {
+  // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
----------------
sammccall wrote:
> 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:
>  - we may not want to make the logic too elaborate until we're able to merge interfaces
>  - we may want to consume AST nodes rather than CCS in the long run
I pulled `CodeCompletionString` handling logic into a separate file in `D41450`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41345