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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks good, just one major drawback to address. We shouldn't break clients by default (see comment in `ClangdMain.cpp`).
And a minor comment about flag naming.



================
Comment at: clangd/tool/ClangdMain.cpp:30
+static llvm::cl::opt<bool>
+    DisableSnippets("disable-snippets",
+                    llvm::cl::desc("Disable \"snippet\" completions and only "
----------------
Maybe replace a negated flag with positive alternative, e.g. `enable-snippets` or `use-snippets`?
Reading such "implicit" double negations is sometimes confusing (e.g. `!DisableSnippets` instead of `EnableSnippets`).


================
Comment at: clangd/tool/ClangdMain.cpp:33
+                                   "present plaintext completions."),
+                    llvm::cl::init(false));
+
----------------
After putting some thought into it, let's disable snippets by default. Sorry for driving this in a wrong direction in my previous comments.
Instead of using flag, LSP servers should look at initial `initialize` request and a value of `TextDocumentClientCapabilities.completion.completionItem.snippetSupport`.
(see https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#initialize-request).
And it seems bad from clangd side to break clients that don't support snippets **by default**. It's probably ok if user passed custom command-line arguments, overriding the defaults.

The problem is that we don't parse the `ClientCapabilities` now. It's not hard to add parsing of it, but that should probably be done in a separate commit and I can do this work if you want.
I think it's fine to commit this as is, **defaulting to not return snippets**, and add parsing of `ClientCapabilities` with a follow-up commit.


https://reviews.llvm.org/D37101