sammccall marked 2 inline comments as done. sammccall added inline comments.
================ Comment at: clangd/JSONRPCDispatcher.h:60 +/// Current context must derive from JSONRPCDispatcher::Handler. +void reply(json::Expr &&Result); +/// Sends an error response to the client, and logs it. ---------------- ilya-biryukov wrote: > `reply`'s signature is scary now. > Should we change it to accept something request-specific that can be found in > the `Context` by the clients? WDYT? > > PS we could change it after the patch lands, just wanted to get your opinion > on this Oops, missed this. To me, the scariest part is that "reply" is a very generic name and its parameter is approximately "anything". In principle your suggestion makes sense, but passing `Context::current()` explicitly is plain weird, and passing `Context::current().get(kRequestToken)` seems pretty grungy in the context. I think I'd be more partial to either giving it a more specific name like `replyToLSPRequest` which hints at the current-context requirement, or actually making all our LSP handlers accept a `UniqueFunction<Expected<T>>` and obviating the need for `reply` altogether. Either way, I'd prefer to address this separately (partly because I'm not sure which is best). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42517 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
