[PATCH] D42517: [clangd] Pass Context implicitly using TLS.

[Ilya Biryukov via Phabricator via cfe-commits]

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