[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

[Raoul Wols via Phabricator via cfe-commits]

rwols marked 10 inline comments as done.
rwols added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:321
+
+    // Fill in the label, detail, documentation and insertText fields of the
+    // CompletionItem.
----------------
ilya-biryukov wrote:
> Maybe split writes into `Item.label` and other `Item` fields?
> That would simplify the function, i.e. something like this:
> 
> ```
> // Set Item.label approriately.
> switch (Result.Kind) {
>   case RK_Declaration: Item.label = Result.Declaration->getNameAsString();
>   //... other enum value
> }
> 
> 
> // Set other Item fields.
> // Note that is also works as expected for RK_Pattern.
> auto Completion = Result.CreateCodeCompletionString(/*....*/);
> ProcessCodeCompleteString(/*...*/, Item);
> // ....
> ```
>From some experimentation and skimming over SemaCodeComplete.cpp I notice the result of `CreateCodeCompletionString(/*....*/)` is never null anyway, so one can just skip those switch cases. I'm not sure why a pointer is returned.


================
Comment at: clangd/ClangdUnit.cpp:381
+        // a declarator or macro.
+        Item.label += Chunk.Text;
+        Item.insertText += Chunk.Text;
----------------
ilya-biryukov wrote:
> We set the label in `ProcessCodeCompleteResult`, why is there a need to append to it here?
> Maybe we should set it here and not touch it in `ProcessCodeCompleteResult` at all?
It's possible that maybe too much is going into the label now, I agree. For instance for a function the name of the function as well as the opening parenthesis, all of its parameters, and the closing brace go into the label. Maybe it's enough to only have the name of the function in the label. On the other hand, I don't think this will play nicely with VSCode and SublimeText, but this will ultimately depend on the language client plugins.


================
Comment at: clangd/ClangdUnit.cpp:406
+        // but should not be inserted into the buffer.
+        Item.documentation += "[" + std::string(Chunk.Text) + "] ";
+        break;
----------------
ilya-biryukov wrote:
> Let's also add some description to `Item.documentation`. What kind of things go into `CK_Informative` ?
> 
I've seen " const", " volatile" go into CK_Informative (note the prefix space). Another major chunk that goes into CK_Informative is the base class name suffixed with `::` if the completion is a method of the base class not overridden in the current class.


================
Comment at: clangd/ClangdUnit.cpp:413
+        break;
+      case CodeCompletionString::CK_CurrentParameter:
+        // A piece of text that describes the parameter that corresponds
----------------
ilya-biryukov wrote:
> How is `CK_CurrentParameter` different from `CK_Placeholder`? Maybe handle them both simultaneously and add a comment on why we don't care about their differences?
> I mean something like:
> ```
> // We don't distinguish between CK_Placeholder and CK_CurrentParameter because ....
> case CodeCompletionString::CK_Placeholder:
> case CodeCompletionString::CK_CurrentParameter:
> //... Code adding ${N:something}
> 
> ```
I'm still figuring out exactly what the difference is...


================
Comment at: clangd/ClangdUnit.cpp:452
+        Item.insertText += Chunk.Text;
+        Item.label += Chunk.Text;
+        break;
----------------
ilya-biryukov wrote:
> Does adding vertical space to `label` plays well with editors? Maybe we should replace with horizontal space instead?
I'll fix this later.


https://reviews.llvm.org/D37101