[PATCH] D43634: [clangd] Extend textDocument/didChange to specify whether diagnostics should be generated.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice,  this will be useful for at least a couple of editor integrations.



================
Comment at: clangd/Protocol.h:301
+
+  /// If this is not set, diagnostics will be generated for the current version
+  /// or a subsequent one.
----------------
Nit: a little weird to lead with the missing case. Suggest rephrase as:

Forces diagnostics to be generated, or to not be generated. for this version of the file.
If not set, diagnostics are eventually consistent: either they will be provided for this version
or some subsequent one.


================
Comment at: test/clangd/want-diagnostics.test:5
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void main() {}"}}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+---
----------------
I think this test doesn't test anything useful because the check lines are not sequenced with respect to the input.

I'm not sure a lit test is that useful here, the actual logic is already unit tested. If we really want to test the ClangdLSPServer change, a unit test might be easier to get right, but personally I'd be happy enough leaving the logic untested (and maybe throwing a wantDiagnostics into one of the updates in protocol.test)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43634