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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
