ilya-biryukov added a comment.

I have left a few comments, but wanted to start with a higher-level design 

I believe we should probably expose the cancellations in the ClangdServer API 
The reasons for that are:

1. We have an internal client that would also benefit from using the 
cancellation API and we should try to share as much code as possible between 
that and the rest of clangd.  Creating the cancellation cookies, stashing them 
in the context are all details that could be handled by ClangdServer so two 
clients don't have to do the same things.
2. `ClangdServer` seems like a natural layer for adding this, since it's a 
user-facing **async** API. It spawns async tasks, so it is a perfect place to 
provide a way to cancel the spawned tasks as well.

The concrete API that I propose is:

- Relevant methods of ClangdServer (we could start we code completion) that 
spawn async reqeuests returns a cancellation object that allows to cancel the 
spawned job:

  class TaskHandle {
    void cancel();
  class ClangdServer {
    TaskHandle codeComplete(File, Position, Callback);

- Users of the API are free to ignore the `TaskHandle` if they don't need to 
cancel the corresponding requests.
- If the users are capable of cancelling the request, they are responsible for 
keeping the `TaskHandle` around before the corresponding callback is called.
- If the request was cancelled, the `Callback` might receive the 
`TaskCancelledError` in the callback and should handle it accordingly.

The rest of the design LG, i.e.  the code that can actually be cancelled is 
responsible for checking `CancellationHandler::HasCancelled` and should return 
a corresponding error in the callback if it was cancelled.

Comment at: clangd/Cancellation.h:1
+//===--- Cancellation.h 
This file could use some comments about the API and samples of how it can be 
Could probably be done after we finalize the design, though.

Comment at: clangd/Cancellation.h:22
+  static bool HasCancelled();
+  static Context LLVM_NODISCARD SetCurrentCancellationToken(
I think we might want to expose the context key for the cancellation primitive 
The reason is that checking if request is cancelled is a common operation (i.e. 
I can imagine it being called on every few parsed ast node, etc.), so we'd want 
to keep the overhead of checking for cancellation very low.

Exposing a key in the context would allow to avoid doing the lookups in the 
context every time we need to check if request was cancelled.

Note that exposing `shared_ptr<atomic<bool>>` is probably not a great idea, 
since it'll allow to write into that object too. I suggest we go with a class 
that wraps it and only allows to read the value of the variable, i.e. only has 
`isCancelled` method.

Comment at: clangd/Cancellation.h:23
+  static bool HasCancelled();
+  static Context LLVM_NODISCARD SetCurrentCancellationToken(
+      std::shared_ptr<std::atomic<bool>> CancellationToken);
The `LLVM_NODISCARD` is misplaced. The current code applies the attribute to 
the type (i.e. `Context`), and we want to apply to the function.
There are two ways to do that:
1. `LLVM_NODISCARD static Context SetCurrentCancellationToken()`
2. `static Context SetCurrentCancellationToken LLVM_NODISCARD ()`

The first one is definitely less awkward and more widely used, so I suggest we 
stick to it

Comment at: clangd/Cancellation.h:25
+      std::shared_ptr<std::atomic<bool>> CancellationToken);
+  static llvm::Error GetCancellationError();
NIT: use `lowerCamelCase` for function names (per [LLVM style 

Comment at: clangd/Cancellation.h:36
+  std::error_code convertToErrorCode() const override {
+    llvm_unreachable("Tried to get error code on TaskCancelledError");
+  }
It seems the contract for `ErrorInfo` actually requires this function to be 
implemented, so using `llvm_unreachable` doesn't seem right here.
We could get away without actually unique error codes by returning 
`llvm::inconvertibleErrorCode()`, though.

Comment at: clangd/ClangdLSPServer.cpp:76
+  return ID.kind() == json::Value::Number
+             ? utostr(static_cast<int64_t>(ID.getAsNumber().getValue()))
+             : std::string(ID.getAsString().getValue());
1. Maybe use `getAsInteger()` to convert directly to `int64_t`?
2. Maybe use `itostr` to avoid conversion from `int64` to `uint64`, which is 
undefined for negative numbers?
3. What if input is neither string nor number? Maybe add an assertion and check 
at the call-sites? That's a form of user input and we don't want clangd to 
crash in case of incorrect ones.

Alternatively, could we simply pipe the `json::Value` to a `raw_string_stream` 
and get the pretty-printed string? Would avoid dealing with input validation 

Comment at: clangd/ClangdLSPServer.h:170
+  // Holds cancellation tokens for requests.
+  llvm::StringMap<std::shared_ptr<std::atomic<bool>>> CancellationTokens;
Maybe clarify in the comment that the key of the map is a serialized request-id?

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to