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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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:
> 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?


================
Comment at: clangd/JSONRPCDispatcher.h:55
+  /// Logs a message locally only. A newline will be added.
+  void log(const Twine &Message) { Out.log(Message + "\n"); }
+
----------------
It seems there are no any of `log`. Maybe remove it from this class?


https://reviews.llvm.org/D38464