[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;
----------------
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.



================
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;
----------------
Maybe there's a way to get rid of `NoParams`?
E.g. by adding a overload to `HandlerRegisterer`?


================
Comment at: clangd/Protocol.cpp:317
+      return ParseFailure();
+    ;
 
----------------
Why do we need these empty `;` statements?


================
Comment at: clangd/ProtocolHandlers.cpp:14
 #include "DraftStore.h"
-using namespace clang;
-using namespace clangd;
----------------
Maybe move this into a separate commit that would also replace `using namespace` in all other files?


================
Comment at: clangd/ProtocolHandlers.cpp:25
+  template <typename Param>
+  void operator()(StringRef method,
+                  void (ProtocolCallbacks::*Handler)(RequestContext &, Param)) {
----------------
Code style: parameter name should be `Method`


https://reviews.llvm.org/D38464