[PATCH] D73218: [clangd] Show background index status using LSP 3.15 work-done progress notifications

[Sam McCall via Phabricator via llvm-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:383
+
+llvm::json::Value toJSON(const WorkDoneProgressBegin &P) {
+  llvm::json::Object Result{
----------------
kadircet wrote:
> why not  have a single struct with that has a required `kind` field and a bunch of optional fields.
> Later on we can assert on the existence of fields depending on the kind, I think it would simplify the implementation.
Yeah, this was my first thought/attempt. It's possible but I think not ideal in the end.

The main downsides:
 - this would diverge from LSP and so make it harder to cross-reference things
 - this would diverge from LSP and usually we don't, so it's surprising
 - the semantics of the "same" field across different events are sometimes subtly different. e.g. Begin.cancellable is whether a cancel button is shown (and in VSCode, the overall UI!) whereas End.cancellable controls its enablement.
 - between replicating the LSP docs, documenting divergence, documenting varied optionality, and documenting varying semantics, the merged struct is really hard to document well
 - it's a bit harder to see the structure of the messages clangd sends this way: e.g. we always send message in report, but never in begin/end, how we handle percentage etc

Mostly I think following LSP closely is a good thing, but it sucks when the spec is a mess :-(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73218/new/

https://reviews.llvm.org/D73218