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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/Index.h:92
 
+  // Documentation including comment for the symbol declaration.
+  std::string Documentation;
----------------
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?


================
Comment at: clangd/index/Index.h:99
+  // Detail about the symbol. For example, the result type of a function.
+  std::string CompletionDetail;
+  // The placeholder text for function parameters in order.
----------------
What are you planning to put here other than the return type of a function?
It's probably OK if we explicitly want this to be "whatever should go in the LSP detail field" but we should think about whether there's something more specific we can say.


================
Comment at: clangd/index/Index.h:100
+  std::string CompletionDetail;
+  // The placeholder text for function parameters in order.
+  std::vector<std::string> Params;
----------------
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.


================
Comment at: clangd/index/SymbolCollector.cpp:61
+
+std::string getDocumentation(const CodeCompletionString &CCS) {
+  // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
----------------
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


================
Comment at: clangd/index/SymbolCollector.cpp:67
+  if (AnnotationCount > 0) {
+    Result += "Annotation";
+    if (AnnotationCount == 1) {
----------------
Should these annotations go at the end?


================
Comment at: clangd/index/SymbolCollector.cpp:90
+
+void ProcessChunks(const CodeCompletionString &CCS, Symbol *Sym) {
+  for (const auto &Chunk : CCS) {
----------------
nit: lowercase


================
Comment at: clangd/index/SymbolCollector.cpp:90
+
+void ProcessChunks(const CodeCompletionString &CCS, Symbol *Sym) {
+  for (const auto &Chunk : CCS) {
----------------
sammccall wrote:
> nit: lowercase
How does this relate to the code in AST-based completion?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41345