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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
