<div dir="ltr">I mention it only out of interest of deduplication of functionality/code within the LLVM project as a whole. Be nice not to maintain two things if one would suffice.<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Oct 12, 2017 at 6:21 AM Sam McCall <<a href="mailto:sammccall@google.com">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Interesting - this is pretty primitive, and still fairly tightly coupled to JSON-RPC.<div>I can't easily tell from the code how the ORC RPC functionality - would it be easy to use with JSON-RPC, does it make sense to use serialization only, does it have opinions about threading models? And really, what would we expect to gain vs using the current more naive code (it looks more complicated, probably having more complex requirements).</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 9, 2017 at 11:16 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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><div class="m_-512081229725485013h5"><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" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_-512081229725485013h5">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></div></div>
_______________________________________________<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>
</blockquote></div><br></div>
</blockquote></div></div>