ilya-biryukov 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. ---------------- sammccall wrote: > 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). Totally agree, let's address it separately. 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
