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