kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/Protocol.cpp:383
+
+llvm::json::Value toJSON(const WorkDoneProgressBegin &P) {
+ llvm::json::Object Result{
----------------
sammccall wrote:
> 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 :-(
> this would diverge from LSP and so make it harder to cross-reference things
ah sorry, I didn't know LSP defined those in 3 different structs, that makes
sense.
SG then
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73218/new/
https://reviews.llvm.org/D73218
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
