<div dir="ltr">hey Lang (& folks here) any chance there's some overlap between the RPC functionality here and the RPC functionality in ORC that could be deduplicated/refactored?<br><br><div class="gmail_quote"><div dir="ltr">On Fri, Oct 6, 2017 at 5:30 AM Ilya Biryukov via Phabricator via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ilya-biryukov accepted this revision.<br>
ilya-biryukov added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
LGTM.<br>
Note there's a new LSP method handler added upstream (`textDocument/signatureHelp`), we should add it to this change before submitting.<br>
<br>
<br>
<br>
================<br>
Comment at: clangd/ClangdLSPServer.h:47<br>
// Implement ProtocolCallbacks.<br>
- void onInitialize(StringRef ID, InitializeParams IP,<br>
- JSONOutput &Out) override;<br>
- void onShutdown(JSONOutput &Out) override;<br>
- void onDocumentDidOpen(DidOpenTextDocumentParams Params,<br>
- JSONOutput &Out) override;<br>
- void onDocumentDidChange(DidChangeTextDocumentParams Params,<br>
- JSONOutput &Out) override;<br>
- void onDocumentDidClose(DidCloseTextDocumentParams Params,<br>
- JSONOutput &Out) override;<br>
- void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,<br>
- StringRef ID, JSONOutput &Out) override;<br>
- void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,<br>
- StringRef ID, JSONOutput &Out) override;<br>
- void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,<br>
- JSONOutput &Out) override;<br>
- void onCodeAction(CodeActionParams Params, StringRef ID,<br>
- JSONOutput &Out) override;<br>
- void onCompletion(TextDocumentPositionParams Params, StringRef ID,<br>
- JSONOutput &Out) override;<br>
- void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,<br>
- JSONOutput &Out) override;<br>
- void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,<br>
- JSONOutput &Out) override;<br>
+ void onInitialize(Ctx &C, InitializeParams &Params) override;<br>
+ void onShutdown(Ctx &C, NoParams &Params) override;<br>
----------------<br>
sammccall wrote:<br>
> ilya-biryukov wrote:<br>
> > ilya-biryukov wrote:<br>
> > > Maybe there's a way to have a typed return value instead of `Ctx`?<br>
> > > This would give an interface that's harder to misuse: one can't forget to call `reply` or call it twice.<br>
> > ><br>
> > > Here's on design that comes to mind.<br>
> > > Notification handlers could have `void` return, normal requests can return `Optional<Result>` or `Optional<std::string>` (with result json).<br>
> > > `Optional` is be used to indicate whether error occurred while processing the request.<br>
> > ><br>
> > After putting more thought into it, `Ctx`-based API seems to have an advantage: it will allow us to easily implement async responses.<br>
> > E.g., we can run code completion on a background thread and continue processing other requests. When completion is ready, we will simply call `Ctx.reply` to return results to the language client.<br>
> ><br>
> > To make that possible, can we allow moving `RequestContext` and pass it by-value instead of by-ref?<br>
> Yeah I thought about returning a value... it certainly reads more nicely, but I don't think we're ready to do a good job in this patch:<br>
> - return value should be an object ready to be serialized (rather than a JSON string) - I don't want to bring that in scope here, but it might affect the details of the API<br>
> - there's several cases we know about (return object, no reply, error reply) and some we're not sure about (async as you mention - any multiple responses)? I think this needs some design, and I don't yet know the project well enough to drive it.<br>
><br>
> I've switched to passing Ctx by value as you suggest (though it's certainly cheap enough to copy, too).<br>
Yeah, copy is also fine there performance-wise.<br>
<br>
I do think move-only interface fits slightly better. We can check a whole bunch of invariants if `Ctx` is move-only (i.e., that request wasn't dropped without response or `reply` was not called twice).<br>
<br>
<br>
================<br>
Comment at: clangd/ClangdLSPServer.h:48<br>
+ void onInitialize(Ctx &C, InitializeParams &Params) override;<br>
+ void onShutdown(Ctx &C, NoParams &Params) override;<br>
+ void onDocumentDidOpen(Ctx &C, DidOpenTextDocumentParams &Params) override;<br>
----------------<br>
sammccall wrote:<br>
> ilya-biryukov wrote:<br>
> > Maybe there's a way to get rid of `NoParams`?<br>
> > E.g. by adding a overload to `HandlerRegisterer`?<br>
> Even if there was, I think I prefer the regularity (changed this to ShutdownParams - oops!).<br>
><br>
> Otherwise the signature's dependent on some combination of {current LSP, whether we actually implement the options, whether we've defined any extensions}. It's harder to remember, means changing lots of places when these factors change, and complicates the generic code.<br>
><br>
> Maybe I've just spent too long around Stubby, though - WDYT?<br>
All those are good points. We also have only one method that does not have params currently, so it's probably not even worth additional complexity in the implementation.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D38464" rel="noreferrer" target="_blank">https://reviews.llvm.org/D38464</a><br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>