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