[PATCH] D96544: [clangd] Extract binding of typed->untyped LSP handlers to LSPBinder. NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! (I wish outgoing calls were not so different :/)



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:173
     } else {
-      log("unhandled notification {0}", Method);
+      auto Handler = Server.Handlers.NotificationHandlers.find(Method);
+      if (Handler != Server.Handlers.NotificationHandlers.end()) {
----------------
why not keep the old lookup style ? since handlers are unique_functions, checking for null should still suffice (and be cheap)


================
Comment at: clang-tools-extra/clangd/LSPBinder.h:67
+  /// e.g. Bind.command("load", this, &ThisModule::load);
+  /// Handler should be e.g. void peek(const LoadParams&, Callback<LoadResult>);
+  /// LoadParams must be JSON-parseable and LoadResult must be serializable.
----------------
s/peek/load


================
Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:18
+
+using testing::_;
+using testing::HasSubstr;
----------------
please fix


================
Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:20
+using testing::HasSubstr;
+using testing::Pair;
+using testing::UnorderedElementsAre;
----------------
please fix


================
Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:25
+struct Foo {
+  int x;
+};
----------------
please fix (we usually follow camelCase for json-serializable types in clangd, but i don't think it is worth doing in tests)


================
Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:28
+bool fromJSON(const llvm::json::Value &V, Foo &F, llvm::json::Path P) {
+  return fromJSON(V, F.x, P);
+}
----------------
nit: s/P/P.field("x")


================
Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:51
+    void notify(const Foo &Params) { lastNotify = Params.x; }
+    int lastNotify = -1;
+  };
----------------
please fix, and maybe have a counter, in addition to last value to check for "invalid type" case?


================
Comment at: clang-tools-extra/clangd/unittests/LSPBinderTests.cpp:69
+  RawPlusOne(1, capture(Reply));
+  EXPECT_THAT_EXPECTED(Reply.getValue(), llvm::HasValue(2));
+  RawPlusOne("foo", capture(Reply));
----------------
nit: `ASSERT_THAT(Reply.hasValue())`

here and elsewhere


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96544/new/

https://reviews.llvm.org/D96544