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 <sammcc...@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 <dblai...@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@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