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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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
----------------
sammccall wrote:
> 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.
> 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).
Our completion lit test include empty capabilities, we don't test it there at all.

> But really I think this can be a unit test in CodeComplete tests instead, right?
We already have a test for that in CodeComplete, but I think we should have a small integration test that cover all three cases where snippetSupport is on, off and missing.
We could sneak in the "missing" case into the existing `completion.test`, but on/off tests are useful to have from my perspective.

> If you really want to verify how missing snippetSupport parses, that's a unittest for protocol.h right? But it doesn't seem important.
I'm not sure checking the parsing code makes sense (it's close to being "correct by construction" from my point of view). Checking that LSP outputs the right thing seems useful, though.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43229