[PATCH] D43229: [clangd] Enable snippet completion based on client capabilities.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/tool/ClangdMain.cpp:60
 
-static llvm::cl::opt<bool> EnableSnippets(
-    "enable-snippets",
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > Would we still have a way to disable snippets (e.g. for debugging) even if clients support them? Maybe make this a hidden option instead?
> I'm tempted to say in that case we could just recompile clangd so that it ignores the options (i.e. change `onInitialized`). The code that will take it into account would be a bit trickier (you can't just pass `CodeCompleteOptions` to `ClangdLSPServer`, we'll also have to pass `Optional<bool> OverridenEnableSnippets`).
+1 to this - this is really a debugging option, and supporting it has a cost.

If we want debugging options to live forever (may be reasonable!) we need a more scalable way to define them than listing them all in ClangdMain.


================
Comment at: test/clangd/completion-snippets-disabled.test:1
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+# RUN: clangd -lit-test -pch-storage=memory < %s | FileCheck -strict-whitespace %s
----------------
Please don't add three new lit tests for this. One should be certainly enough (off, assuming our normal completion test has this feature on). 

But really I think this can be a unit test in CodeComplete tests instead, right?

If you really want to verify how missing `snippetSupport` parses, that's a unittest for protocol.h right? But it doesn't seem important.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43229