This revision was automatically updated to reflect the committed changes.
Closed by commit rL340607: [clangd] Initial cancellation mechanism for LSP
requests. (authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
kadircet updated this revision to Diff 162365.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Resolve comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
clangd/Cancellation.cpp
clangd/Cancellation.h
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LG with a few small comments
Comment at: clangd/Protocol.cpp:635
+ if(const auto AsNumber = Params->getAsInteger())
+return utostr(AsNumber.getValue());
+
kadircet updated this revision to Diff 162354.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Change helper for normalizing Number | String.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
clangd/Cancellation.cpp
kadircet added inline comments.
Comment at: clangd/Protocol.h:883
+/// converts it into a string.
+bool parseNumberOrString(const llvm::json::Value , std::string ,
+ const std::string );
ilya-biryukov wrote:
> Maybe simplify the signature
kadircet marked 2 inline comments as done.
kadircet added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:351
+ [this](llvm::Expected List) {
+auto _ = llvm::make_scope_exit([this]() { CleanupTaskHandle(); });
+
ilya-biryukov wrote:
>
kadircet updated this revision to Diff 162342.
kadircet marked 3 inline comments as done.
kadircet added a comment.
- Change a few comments.
- Add some assertions.
- Fix a data race.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
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 () {
+ return *Context::current().getExisting(TaskKey);
+}
Maybe add 'assert' that TaskKey is present?
To make
kadircet updated this revision to Diff 161908.
kadircet marked 6 inline comments as done.
kadircet added a comment.
- Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
clangd/Cancellation.cpp
clangd/Cancellation.h
ilya-biryukov added a comment.
LG, just a few last nits
Comment at: clangd/Cancellation.cpp:30
+
+TaskHandle createTaskHandle() { return std::make_shared(); }
+
NIT: maybe consider inlining it? Since Task has a public default constructor, I
don't think having
kadircet updated this revision to Diff 161716.
kadircet marked 4 inline comments as done.
kadircet added a comment.
- Get rid of CancellationToken && Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
ilya-biryukov added inline comments.
Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > As chatted offline, I have
ioeric added inline comments.
Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:
ilya-biryukov wrote:
> ioeric wrote:
> > As chatted offline, I have questions about the motivation
ilya-biryukov added inline comments.
Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:
ioeric wrote:
> As chatted offline, I have questions about the motivation of the
>
kadircet added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:75
+std::string NormalizeRequestID(const json::Value ) {
+ CancelParams CP;
+ fromJSON(json::Object{{"id", ID}}, CP);
ioeric wrote:
> Use `ID.getAsString()`?
Unfortunately id can be both a
ioeric added inline comments.
Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:
As chatted offline, I have questions about the motivation of the
`CancellationToken` class.
kadircet updated this revision to Diff 161428.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Check cancellation earlier && Fix normalization difference between string and
integer request ids.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:80
+ if (NormalizedID.front() == '"')
+NormalizedID = NormalizedID.substr(1, NormalizedID.size() - 2);
+ return NormalizedID;
This still misses string escaping issues. E.g. `"`
kadircet marked an inline comment as done.
kadircet added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:621
+ std::lock_guard Lock(TaskHandlesMutex);
+ const auto = TaskHandles.find(Params.ID);
+ if (it != TaskHandles.end()) {
ilya-biryukov wrote:
kadircet updated this revision to Diff 161261.
kadircet marked 10 inline comments as done.
kadircet added a comment.
- Resolve discussions & Delete enclosing quotes when normalizing.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
ilya-biryukov added a comment.
Mostly NITs, but please take a look at the `CancelParams` handling problem. I
believe there might be a potential bug hiding there :-)
Comment at: clangd/Cancellation.h:87
+/// Enables async tasks to check for cancellation signal, contains a read
kadircet updated this revision to Diff 161018.
kadircet marked 21 inline comments as done.
kadircet added a comment.
- Resolve discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
clangd/Cancellation.cpp
clangd/Cancellation.h
kadircet added inline comments.
Comment at: clangd/Cancellation.h:92
+ operator bool() const { return isCancelled(); }
+ friend CancellationToken isCancelled();
+
ilya-biryukov wrote:
> It's a bit confusing that this name clashes with a member function.
> We
ilya-biryukov added a comment.
The last set of comments is for the previous version, might be missing some
updates, sorry about that. Will make sure to review the one too!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
___
ilya-biryukov added a comment.
Mostly LSP-specific comments and comments about making TaskHandle thread-safe.
I'm also starting to wonder if the scope of this patch is too big, we could
potentially split it into three separate bits:
1. Generic cancellation API, i.e. `Cancellation.h` and
kadircet updated this revision to Diff 160986.
kadircet added a comment.
- Made TaskHandle move-only. Since it is costly and most likely unnecessary to
copy it other than to move it into Context.
- Provided an explicit clone method for copying.
Repository:
rCTE Clang Tools Extra
kadircet added inline comments.
Comment at: clangd/Cancellation.h:71
+
+class TaskHandle {
+public:
ilya-biryukov wrote:
> I wonder if we should make `TaskHandle` move-only?
>
> The reasoning is that is can be easily used from multiple threads (since it's
>
kadircet updated this revision to Diff 160652.
kadircet marked 8 inline comments as done.
kadircet added a comment.
- Resolve discussions.
- Get rid of CancellationHandler class.
- Change error class name.
- Improve documentation.
Repository:
rCTE Clang Tools Extra
kadircet added inline comments.
Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key>> CancellationTokenKey;
+} // namespace
ilya-biryukov wrote:
> Having a `shared_ptr` key in the Context can cause data races (e.g. if we
> copy it concurrently from
kadircet updated this revision to Diff 160636.
kadircet marked 4 inline comments as done.
kadircet added a comment.
- Get rid of getCancellationError.
- Add replyError helper.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
kadircet updated this revision to Diff 160531.
kadircet marked 7 inline comments as done.
kadircet added a comment.
- Polished API.
- Resolved discussions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
clangd/Cancellation.cpp
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
kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jfb, arphaman, jkorous, MaskRay, ioeric,
mgorny.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
Files:
clangd/CMakeLists.txt
clangd/Cancellation.cpp
33 matches
Mail list logo
