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@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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
