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