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

[David Blaikie via cfe-commits]

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.

On Thu, Oct 12, 2017 at 6:21 AM Sam McCall <sammccall at google.com> wrote:

> 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/1f8f1918/attachment.html>