ilya-biryukov added a comment.
I have left a few comments, but wanted to start with a higher-level design
consideration.
I believe we should probably expose the cancellations in the ClangdServer API
directly.
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
-------------------------------------------*-C++-*-===//
+//
----------------
This file could use some comments about the API and samples of how it can be
used.
Could probably be done after we finalize the design, though.
================
Comment at: clangd/Cancellation.h:22
+public:
+ static bool HasCancelled();
+ static Context LLVM_NODISCARD SetCurrentCancellationToken(
----------------
I think we might want to expose the context key for the cancellation primitive
here.
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
guide](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly))
================
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
issues.
================
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?
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
