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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks good overall, just a bunch of comments before accepting the revision. Mostly minor code style issues, sorry for spamming you with those.



================
Comment at: clangd/ClangdLSPServer.h:30
   ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
+                  const bool SnippetCompletions,
                   llvm::Optional<StringRef> ResourceDir);
----------------
Code style: we don't use `const` for non-reference parameters.


================
Comment at: clangd/ClangdUnit.cpp:275
 
+std::string SnippetEscape(const llvm::StringRef Text) {
+  std::string Result;
----------------
Code style: function names are `lowerCamelCase`.
Also maybe rename to `escapeSnippet` to follow the style guide: "function names should be verb phrases, e.g. `openFile()` or `isFoo()`" (see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)


================
Comment at: clangd/ClangdUnit.cpp:315
+protected:
+  std::vector<CompletionItem> &Items;
+  std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
----------------
Code style: we put all field definitions at the bottom of the class in clangd.
Also: why not make the fields private?



================
Comment at: clangd/ClangdUnit.cpp:376
+
+    CompletionItem Item{InsertTextFormat::PlainText};
+
----------------
Implementations of this function in `PlainTextCompletionItemsCollector` and `SnippetCompletionItemsCollector` are the same.
Maybe make `ProcessChunks` virtual instead?

Or maybe consider replacing inheritance with a `bool` flag. Inheritance does not seem to buy us much here. This looks simpler:
```
if (EnableSnippets)
  ProcessChunksWithSnippets(CCS, Item);
else
  ProcessChunksWithoutSnippets(CCS, Item);

```




================
Comment at: clangd/ClangdUnit.cpp:454
+        // a declarator or macro.
+        Item.filterText = Chunk.Text;
+      case CodeCompletionString::CK_Text:
----------------
Maybe add a comment that fallthrough to the next case is intended?
It's very easy to miss that there's no break intentionally when reading the code.


================
Comment at: clangd/ClangdUnit.cpp:533
+        Item.insertText += Chunk.Text;
+        // Don't even add a space to the label.
+      }
----------------
Maybe add `break` here?
To avoid unintentional fallthrough if new `case` statement gets added.


================
Comment at: clangd/Protocol.h:393
+  CompletionItem() = default;
+  CompletionItem(const InsertTextFormat Format);
+
----------------
Maybe remove these constructors?
It seems weird that `insertTextFormat` is initialized on construction, while all other fields are filled up later.
Having to write all the fields explicitly is pretty low-level, but it's consistent and is done in all other structs from `Protocol.h`.


================
Comment at: clangd/tool/ClangdMain.cpp:31
+static llvm::cl::opt<bool> EnableSnippets(
+    "enable-snippets",
+    llvm::cl::desc("Present completions with embedded snippets instead of "
----------------
I've managed to get snippet completion working in VSCode. 
The problem is that we're using outdated version of `vscode-languageclient` dependency in our npm module.
Simply bumping versions of dependencies in `clangd\clients\clangd-vscode\package.json` worked.
Had to change
```
    "dependencies": {
        "vscode-languageclient": "^2.6.3",
        "vscode-languageserver": "^2.6.2"
    },
```
to 
```
    "dependencies": {
        "vscode-languageclient": "^3.3.0",
        "vscode-languageserver": "^3.3.0"
    },
```

Maybe include this in this commit and make `-enable-snippets` true by default?


https://reviews.llvm.org/D37101