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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 16 06:36:56 PDT 2018


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 seem to optimize for making this work anywhere:
> ```
>   if (isCancelled())  // <-- get token from ctx, then use implicit conversion to check if it was cancelled
>     return llvm::makeError<CancelledError>();
> ```
> 
> Which is probably a nice pattern. But we could have two functions that do the same thing with less magic (i.e. implicit conversions) involved at the call sites:
> ```
> CancellationToken getCurrentCancellation();
> void setCurrentCancellation(CancellationToken CT);
> 
> inline bool isCancelled() { return getCurrentCancellation().isCancelled(); }
> ```
> Would allow to get rid of implicit conversion and friend function with the same signature as member. Would also make cases where we actually want to stash the token somewhere cleaner, e.g. instead of `CancellationToken CT = isCancelled()`, we'll have `CancellationToken CT = getCurrentCancellation()`.
> WDYT?
Totally agreed, actually, didn't even notice they had the same signature :D, thanks!
 But I still think implicit conversion of CancellationToken to bool seems like a good thing to have.


================
Comment at: clangd/ClangdLSPServer.h:179
+  // associated with only their thread.
+  void CleanupTaskHandle();
+  void StoreTaskHandle(TaskHandle TH);
----------------
ilya-biryukov wrote:
> These two methods look like a nice suit for better wrapped in a RAII-object. It would add the value to the associated map on construction and remove on destruction.
> One problem is that all operations calls would become weird, because we'll have to move this RAII object around and moving into lambdas is not fun without C++14 (which allows `[x=std::move(y)]() {}`..
> 
> Maybe add a comment that calls to this function should always be paired and a FIXME that we should have an API that enforces this?
> 
Yeah that would be really nice to have them in a RAII object, but the thing is cleanup can occur only after the task dies. Which we don't directly know since threads are being detached. So the only way to trigger destruction is at the callback, which is where I call cleanup currently. Maybe we can store the object within context to trigger destructor at the end of the thread, but still don't think it would look any better or ease the job of the caller.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502





More information about the cfe-commits mailing list