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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

LG, just a few last nits



================
Comment at: clangd/Cancellation.cpp:30
+
+TaskHandle createTaskHandle() { return std::make_shared<Task>(); }
+
----------------
NIT: maybe consider inlining it? Since Task has a public default constructor, I don't think having this method buys us anything


================
Comment at: clangd/Cancellation.h:22
+//  function<void(llvm::Expected<ResultType>)> Callback) {
+//   // Make sure Taskr is created before starting the thread. Otherwise
+//   // CancellationToken might not get copied into thread.
----------------
s/Taskr/Task


================
Comment at: clangd/Cancellation.h:23
+//   // Make sure Taskr is created before starting the thread. Otherwise
+//   // CancellationToken might not get copied into thread.
+//   auto TH = createTaskHandle();
----------------
remove mentions of CancellationToken


================
Comment at: clangd/Cancellation.h:49
+// The worker code itself (3) should check for cancellations using
+// `CancellationToken` that can be retrieved via
+// `CancellationToken::isCancelled()`.
----------------
s/CancellationToken/getCurrentTask().isCancelled()


================
Comment at: clangd/ClangdLSPServer.cpp:76
+  CancelParams CP;
+  fromJSON(json::Object{{"id", ID}}, CP);
+  return CP.ID;
----------------
Maybe extract the relevant code into a helper to avoid creating the unrelated class (`CancelParams`)?


================
Comment at: clangd/ClangdLSPServer.cpp:621
+  const auto &it = TaskHandles.find(Params.ID);
+  if (it != TaskHandles.end()) {
+    it->second->cancel();
----------------
Invert condition to reduce nesting?
See [LLVM Style Guide](https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502