sammccall added a comment.
Thanks! All addressed (except removing ShutdownParams, which I'm not sure is
worthwhile).
================
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;
----------------
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).
================
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;
----------------
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?
https://reviews.llvm.org/D38464
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
