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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:321
+
+    // Fill in the label, detail, documentation and insertText fields of the
+    // CompletionItem.
----------------
rwols wrote:
> 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.
Probably just a way to indicate that memory for returned item is managed by `Allocator`.


================
Comment at: clangd/ClangdUnit.cpp:316
+    // CK_Placeholder or CK_CurrentParameter, this will be modified to
+    // InsertTextFormat::PlainText.
+    Item.insertTextFormat = InsertTextFormat::PlainText;
----------------
Probably meant to be `InsertTextFormat::Snippet`, not `InsertTextFormat::PlainText`.


================
Comment at: clangd/ClangdUnit.cpp:379
+
+        // TODO: Maybe add an option to allow presenting the optional chunks?
+        break;
----------------
Style guide: use FIXME instead of TODO


================
Comment at: clangd/ClangdUnit.cpp:393
+        // Terrible hack.
+        // TODO: See SemaCodeComplete.cpp:2598
+        if (Chunk.Text[0] == ' ')
----------------
Style guide: use FIXME instead of TODO


================
Comment at: clangd/ClangdUnit.cpp:393
+        // Terrible hack.
+        // TODO: See SemaCodeComplete.cpp:2598
+        if (Chunk.Text[0] == ' ')
----------------
ilya-biryukov wrote:
> Style guide: use FIXME instead of TODO
Maybe reference function name instead of the line number? Line numbers change too often.


================
Comment at: clangd/ClangdUnit.cpp:395
+        if (Chunk.Text[0] == ' ')
+          Item.documentation += "[" + std::string(Chunk.Text + 1) + "] ";
+        else
----------------
After looking at code completion code, it seems that informative chunks are parts of function signature (that have custom highlightings in XCode, probably).
Maybe we could put them into label? It feels that `const` belongs to the parameter list and we put parameter list into `label` now.
That would also make the `Chunk.Text + 1` part unnecessary.


================
Comment at: clangd/ClangdUnit.cpp:452
+
+  void AddSnippetParameter(const char *Text, unsigned &ArgCount,
+                           CompletionItem &Item) const {
----------------
Maybe use `StringRef` instead of `const char*`?


================
Comment at: clangd/ClangdUnit.cpp:461
+
+  std::string SnippetEscape(const char *Text) const {
+    std::string Result;
----------------
Maybe use `StringRef` instead of `const char*`?


https://reviews.llvm.org/D37101