sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:278
~ReplyOnce() {
- if (Server && !Replied) {
+ if (Server && !Server->isDestructing() && !Replied) {
elog("No reply to message {0}({1})", Method, ID);
----------------
sammccall wrote:
> Need a comment about the server being destroyed/unreplied calls case.
You've added a comment that repeats what the code does, please don't do that :-)
Instead, explain the context around this case. e.g.
"There's one legitimate reason to never reply to a request: clangd's request
handler sent a call to the client (e.g. applyWorkspaceEdit) and the client
never replied. In this case, the ReplyOnce is owned by ClangdServer's reply
callback table and is destroyed along with the server. We don't attempt to send
a reply in this case, there's little to be gained from doing so."
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:205
+ // Return a call id of the request.
+ int bindReply(Callback<llvm::json::Value> Reply) {
+ llvm::Optional<std::pair<int, Callback<llvm::json::Value>>> OldestCB;
----------------
nit: I think this function could return json::Value, encapsulating the fact
that we use integers inside the MessageHandler class.
(Or does something outside the class depend on this?)
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:228
+ OldestCB->first)));
+ assert(ID != -1 && "ID must be set");
+ return ID;
----------------
There's no complicated flow control around this - I'd suggest instead leaving
ID uninitialized and letting msan flag this if it goes wrong.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:317
llvm::StringMap<std::function<void(llvm::json::Value, ReplyOnce)>> Calls;
+ // The maximum number of callbacks hold in clangd.
+ //
----------------
"held" or "to hold"
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:321
+ // for cases where LSP clients don't reply for the request.
+ static constexpr int MaxCallbacks = 100;
+ mutable std::mutex CallMutex;
----------------
more specific name: MaxReplyCallbacks or even better MaxOutstandingCalls?
================
Comment at: clang-tools-extra/clangd/test/fixits-command.test:206
---
+{"jsonrpc":"2.0","id":0,"result":{"applied":true}}
+# CHECK: "id": 4,
----------------
this ID is reused
================
Comment at: clang-tools-extra/clangd/test/request-reply.test:28
+---
+{"jsonrpc":"2.0","id":4,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"file:///clangd-test/main.cpp","selection":{"end":{"character":4,"line":0},"start":{"character":0,"line":0}},"tweakID":"ExpandAutoType"}]}}
+# CHECK: "id": 1,
----------------
ID reuse here (4 was previously used on line 6)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65387/new/
https://reviews.llvm.org/D65387
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
