[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly NITs, but please take a look at the potential data race



================
Comment at: clangd/Cancellation.cpp:23
+const Task &getCurrentTask() {
+  return *Context::current().getExisting(TaskKey);
+}
----------------
Maybe add 'assert' that TaskKey is present?
To make sure we see the breakages as early as possible when asserts are enabled (otherwise it's undefined behavior and it can propagate to breakages in client code)


================
Comment at: clangd/Cancellation.h:27
+//   }
+//   // Start run() in a new async thread, and make sure to propagete Context.
+//   return TH;
----------------
s/propagete/propagate


================
Comment at: clangd/Cancellation.h:114
+
+/// Fetches current task information from Context.
+const Task &getCurrentTask();
----------------
Maybe add a comment that the current `Context` must be have the task stashed in it?


================
Comment at: clangd/ClangdLSPServer.cpp:351
+      [this](llvm::Expected<CodeCompleteResult> List) {
+        auto _ = llvm::make_scope_exit([this]() { CleanupTaskHandle(); });
+
----------------
CleanupTaskHandle() can run in a separate thread, so can potentially run before the `StoreTaskHandle` call.
To avoid memory leaks in that case, let's preallocate the entry **before** calling `codeComplete`


================
Comment at: clangd/ClangdLSPServer.cpp:621
+  std::lock_guard<std::mutex> Lock(TaskHandlesMutex);
+  const auto &it = TaskHandles.find(Params.ID);
+  if (it == TaskHandles.end())
----------------
s/it/It


================
Comment at: clangd/Protocol.cpp:629
+                         const std::string &Field) {
+  json::ObjectMapper O(Params);
+  if (!O)
----------------
Maybe switch on different kinds of `json::Value` instead of using the `ObjectMapper`?
The code should be simpler. Albeit the `fromJson` of `CancelParams` might get a little more complicated, but that looks like a small price to pay.


================
Comment at: clangd/Protocol.h:883
+/// converts it into a string.
+bool parseNumberOrString(const llvm::json::Value &Params, std::string &Parsed,
+                         const std::string &Field);
----------------
Maybe simplify the signature to: `Optional<string> parseNumberOrString(const llvm::json::Value&);`
And call as `parseNumberOrString(obj["id"])` instead of `parseNumberOrString(obj, "id")`?

Also, maybe call it `normalizeRequestId`? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502