sammccall added a comment.
Thanks, this looks a lot better. There are a bunch of details that can be
simplified and names that could be clearer, but I think this is the right shape.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:161
+
+ Callback<llvm::json::Value> FoundCB = nullptr;
+ if (auto IntID = ID.getAsInteger()) {
----------------
nit: maybe call this ReplyHandler?
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:165
+ // Find a corresponding callback for the request ID;
+ // We can't use binary search here as ReplyCallbacks is not guaranteed to
+ // be sorted in the multi-thread environment.
----------------
this got me thinking - we're using an `atomic<int>` to assign sequential IDs,
but then locking to put them into a map. The reward for using two types of
concurrency primitives is our array is almost-but-not-quite sorted.
I think we should demote `NextCallID` to a regular int, and guard it with the
same mutex as ReplyCallbacks. (Just call it `CallMutex`?) Then bindReply can
lock the mutex, pick a call id, add the callback to the map, and return the ID.
I still don't think we need to binary search though. The list is 100 at most
and ~1 in practice.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:167
+ // be sorted in the multi-thread environment.
+ auto Found =
+ llvm::find_if(ReplyCallbacks, [&IntID](const ReplyCallback &RCB) {
----------------
nit: a normal loop with if/break seems clearer than find_if here
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:177
+
+ if (FoundCB) {
+ // Found a corresponding callback from the queue.
----------------
Might just be me, but I think I'd find it slightly to avoid the LogAndRun
lambda by overwriting the callback:
```
// If there was no callback in the table, the client is sending us an
unsolicited reply!
if (FoundCB == nullptr)
Found = [&ID](...) { elog and consume error; }
// inline LogAndRun here
```
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:184
+ // No callback being found, run a default log callback.
+ LogAndRun([&ID](decltype(Result) Result) {
+ elog("received a reply with ID {0}, but there was no such call", ID);
----------------
nit: we tend to use `decltype(Result)` to specify the type of `Result` when
we're pseudo-capturing it in a lambda using `Bind()`. That's not the case here,
so please spell out the type.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:218
+ // take out and run the oldest pending callback if we overflow.
+ if (ReplyCallbacks.size() > MaxCallbacks) {
+ OldestCB = std::move(ReplyCallbacks.front());
----------------
elog("more than {0} outstanding LSP calls, forgetting about {1}")
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:224
+ if (OldestCB)
+ OldestCB->CB(
+ llvm::formatv("failed to receive a client reply for request ({0})",
----------------
you're returning a string to the callback, I think you meant to return an error?
================
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);
----------------
Need a comment about the server being destroyed/unreplied calls case.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:315
+ //
+ // We bound the maximum size to the pending queue to prevent memory leakage
+ // for cases where LSP clients don't reply for the request.
----------------
nit: physically this is a deque, but it functions more like a map and I think
it might be easier to refer to that way.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:318
+ //
+ // If the queue overflows, we assume that the client didn't reply the
+ // oldest request, and run the corresponding callback which replies an Error
----------------
nit: I think this part of the comment belongs in the implementation rather than
on the variable
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:325
+ // a corresponding request.
+ struct ReplyCallback {
+ int RequestID;
----------------
hmm, maybe just use `pair<int, Callback<Value>>` here? That way it looks even
more like a map
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:580
};
+ auto ReplyApplyEdit =
+ [](decltype(Reply) Reply,
----------------
I can't parse this name.
Looking at the code... it's doing something like `ReplyAfterApplyingEdits`
It should take the success message as a parameter, I think - at the moment
you're replying "fix applied" even in the tweak case
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:580
};
+ auto ReplyApplyEdit =
+ [](decltype(Reply) Reply,
----------------
sammccall wrote:
> I can't parse this name.
>
> Looking at the code... it's doing something like `ReplyAfterApplyingEdits`
>
> It should take the success message as a parameter, I think - at the moment
> you're replying "fix applied" even in the tweak case
So this method is already a tangled mess, and this patch makes it worse.
However, it's *conceptually* exactly the right API, just callbacks and Expected
and LSP conspire to be awkward here.
We should refactor this, but this patch isn't the right time. Can you add a
`//FIXME: this method is tangled and confusing, refactor it` or so?
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:591
+ llvm::inconvertibleErrorCode(),
+ ("edits fail to applied: " + Reason).c_str()));
+ }
----------------
nit: edits were not applied (or edits failed to apply)
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:54
+ bool isDestructing() const { return Destructing; }
+
----------------
MessageHandler is a nested class and so has access to Destructing already, no
need for this function
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:138
+ std::atomic<bool> Destructing = {false};
+
----------------
nit: Destroying
but I think `IsBeingDestroyed` would be clearer, as destroy is transitive
this needs a comment
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:159
+ // Wrap the callback with LSP conversion and error-handling.
+ auto Reply = [](decltype(CB) CB,
+ llvm::Expected<llvm::json::Value> RawResponse) {
----------------
nit: HandleReply?
lambdas often have names like functions, so this reads like it's a function
that replies
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:174
+ }
+ void callImpl(StringRef Method, llvm::json::Value Params,
+ Callback<llvm::json::Value> CB);
----------------
nit: I know I suggested this name but I think `callRaw` would be better
================
Comment at: clang-tools-extra/clangd/Protocol.h:877
+struct ApplyWorkspaceEditResponse {
+ bool applied;
+ llvm::Optional<std::string> failureReason;
----------------
needs a default value
`= true`, I guess
================
Comment at: clang-tools-extra/clangd/test/request-reply.test:5
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"auto
i = 0;"}}}
+---
+---
----------------
nit, extra line
================
Comment at: clang-tools-extra/clangd/test/request-reply.test:22
+---
+{"jsonrpc":"2.0","id":0,"result":{"applied":false}}
+# CHECK: "code": -32001,
----------------
please use increasing IDs and don't reuse them. (In general, but particularly
in this test)
================
Comment at: clang-tools-extra/clangd/test/request-reply.test:23
+{"jsonrpc":"2.0","id":0,"result":{"applied":false}}
+# CHECK: "code": -32001,
+# CHECK-NEXT: "message": "edits fail to applied:
----------------
can you verify that the error comes with the right ID? (and similar below)
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
