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