kadircet added a comment.
LGTM, mostly nits.
Unfortunately I don't see any way to make it simpler :/
================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:104
+
+MATCHER_P(DiagMessage, M, "") {
+ if (const auto *O = arg.getAsObject()) {
----------------
nit: move this to top of the file
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:65
+
+ void reply(llvm::json::Value ID,
+ llvm::Expected<llvm::json::Value> V) override {
----------------
put methods before fields
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:68
+ if (V)
+ logBody("reply", *V, /*Send=*/false);
+ std::lock_guard<std::mutex> Lock(Mu);
----------------
i think logging errors here could help with debugging.
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:71
+ if (auto I = ID.getAsInteger()) {
+ if (*I >= 0 && *I < (int64_t)CallResults.size()) {
+ CallResults[*I].set(std::move(V));
----------------
nit: static_cast
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:99
+ Lock.unlock();
+ if (!Action) // stop!
+ return llvm::Error::success();
----------------
this check should happen before accessing Actions.front
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:106
+
+public:
+ std::pair<llvm::json::Value, CallResult *> addCallSlot() {
----------------
put public before private
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:157
+
+void LSPClient::stop() { T->enqueue(nullptr); }
+
----------------
ah, the `!Action` check for stop now makes sense :D maybe get rid of the `Stop`
flag inside transportimpl and say `an empty action terminates the loop` or make
this invoke a T->stop(); which sets the flag
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:197
+ auto U = PubDiagsParams->getString("uri");
+ if (!U || *U != uri(Path))
+ continue;
----------------
URI is not an optional field, so shouldn't `!U` -> ADD_FAILURE() ?
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:199
+ continue;
+ if (const auto *Diagnostics = PubDiagsParams->getArray("diagnostics"))
+ return std::vector<llvm::json::Value>(Diagnostics->begin(),
----------------
again `diagnostics` is not optional
================
Comment at: clang-tools-extra/clangd/unittests/LSPClient.h:44
+ friend TransportImpl;
+ void set(llvm::Expected<llvm::json::Value> V);
+ };
----------------
nit: put it before members, also some comments about "multiple call is failure"
might be nice
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77766/new/
https://reviews.llvm.org/D77766
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
