[PATCH] D38464: [clangd] less boilerplate in RPC dispatch

[Sam McCall via cfe-commits]

Interesting - this is pretty primitive, and still fairly tightly coupled to
JSON-RPC.
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).

On Mon, Oct 9, 2017 at 11:16 PM, David Blaikie <dblaikie at gmail.com> wrote:

> 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?
>
> On Fri, Oct 6, 2017 at 5:30 AM Ilya Biryukov via Phabricator via
> cfe-commits <cfe-commits at lists.llvm.org> wrote:
>
>> ilya-biryukov accepted this revision.
>> ilya-biryukov added a comment.
>> This revision is now accepted and ready to land.
>>
>> LGTM.
>> Note there's a new LSP method handler added upstream
>> (`textDocument/signatureHelp`), we should add it to this change before
>> submitting.
>>
>>
>>
>> ================
>> Comment at: clangd/ClangdLSPServer.h:47
>>    // Implement ProtocolCallbacks.
>> -  void onInitialize(StringRef ID, InitializeParams IP,
>> -                    JSONOutput &Out) override;
>> -  void onShutdown(JSONOutput &Out) override;
>> -  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
>> -                         JSONOutput &Out) override;
>> -  void onDocumentDidChange(DidChangeTextDocumentParams Params,
>> -                           JSONOutput &Out) override;
>> -  void onDocumentDidClose(DidCloseTextDocumentParams Params,
>> -                          JSONOutput &Out) override;
>> -  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
>> -                                  StringRef ID, JSONOutput &Out)
>> override;
>> -  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
>> -                                 StringRef ID, JSONOutput &Out) override;
>> -  void onDocumentFormatting(DocumentFormattingParams Params, StringRef
>> ID,
>> -                            JSONOutput &Out) override;
>> -  void onCodeAction(CodeActionParams Params, StringRef ID,
>> -                    JSONOutput &Out) override;
>> -  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
>> -                    JSONOutput &Out) override;
>> -  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
>> -                        JSONOutput &Out) override;
>> -  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
>> -                            JSONOutput &Out) override;
>> +  void onInitialize(Ctx &C, InitializeParams &Params) override;
>> +  void onShutdown(Ctx &C, NoParams &Params) override;
>> ----------------
>> sammccall wrote:
>> > ilya-biryukov wrote:
>> > > ilya-biryukov wrote:
>> > > > Maybe there's a way to have a typed return value instead of `Ctx`?
>> > > > This would give an interface that's harder to misuse: one can't
>> forget to call `reply` or call it twice.
>> > > >
>> > > > Here's on design that comes to mind.
>> > > > Notification handlers could have `void` return, normal requests can
>> return `Optional<Result>` or `Optional<std::string>` (with result json).
>> > > > `Optional` is be used to indicate whether error occurred while
>> processing the request.
>> > > >
>> > > After putting more thought into it, `Ctx`-based API seems to have an
>> advantage: it will allow us to easily implement async responses.
>> > > 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.
>> > >
>> > > To make that possible, can we allow moving `RequestContext` and pass
>> it by-value instead of by-ref?
>> > 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:
>> >  - 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
>> >  - 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.
>> >
>> > I've switched to passing Ctx by value as you suggest (though it's
>> certainly cheap enough to copy, too).
>> Yeah, copy is also fine there performance-wise.
>>
>> 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).
>>
>>
>> ================
>> Comment at: clangd/ClangdLSPServer.h:48
>> +  void onInitialize(Ctx &C, InitializeParams &Params) override;
>> +  void onShutdown(Ctx &C, NoParams &Params) override;
>> +  void onDocumentDidOpen(Ctx &C, DidOpenTextDocumentParams &Params)
>> override;
>> ----------------
>> sammccall wrote:
>> > ilya-biryukov wrote:
>> > > Maybe there's a way to get rid of `NoParams`?
>> > > E.g. by adding a overload to `HandlerRegisterer`?
>> > Even if there was, I think I prefer the regularity (changed this to
>> ShutdownParams - oops!).
>> >
>> > 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.
>> >
>> > Maybe I've just spent too long around Stubby, though - WDYT?
>> 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.
>>
>>
>> https://reviews.llvm.org/D38464
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171012/6a7881b6/attachment.html>