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

[Sam McCall via Phabricator via cfe-commits]

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