[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

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 questions about the motivation of the `CancellationToken` class. Intuitively, it seems to me that this can be merged into `TaskHandle`, and we can simply stash the `TaskHandle` instead of a token into the context. There would be fewer states to maintain, and the design would be a bit more straightforward. I might be missing context/concerns here, so I'd like to hear what you and Ilya think. @ilya-biryukov 
> > I am for splitting cancellation checks from cancellation triggers.
> > The processing code only needs to check if it was cancelled and exposing the `cancel()` does not add any useful functionality, merely ways to misuse it.
> Couldn't we prevent this by passing only const handle references to users who are not expect to call `cancel()`?
As long as `TaskHandle` does not have a copy constructor, that LG.
I.e. the scheme that seems to work is:
1. return `shared_ptr<TaskHandle>` from async computations like code complete.
2. stash `shared_ptr<const TaskHandle>` into the context.
3. return `const TaskHandle*` from the context.

This should give a simpler implementation.
The cons from my point of view are:
1. We return copyable task handles (i.e. the shared_ptr) from the API by default. This provokes usages that don't take ownership issues into account, i.e. it's easy to just copy the task everywhere without having clear ownership of it. As long as it's just cancellation there, we're probably fine, but if we add more stuff into it that might become a problem.
2. We expose implementation details (i.e. shared_ptr) by removing the layer of abstraction. This makes potential refactorings of the client code harder.

In general, I think having our own wrappers there opens up more room for experiments with the API later (the API is very constrained, so it's easy to refactor the implementation).  OTOH, if we believe the design is right or changing the clients is not too much work (which is probably true), either way is fine with me.

But either solution has its pros and cons, I'm happy with trying out any of those on code completion, see how it goes and decide whether we want to change anything before finalizing our design and adding cancellation to all operations in clangd.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502