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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
