[PATCH] D42174: [clangd] Refactored threading in ClangdServer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.h:164
 
+/// Handles running tasks for ClangdServer and managing the resources (e.g.,
+/// preambles and ASTs) for opened files.
----------------
sammccall wrote:
> This is a nice abstraction, so much better than dealing with Cppfile! A couple of observations:
> 
> 1) all methods refer to a single file, neither the contracts nor the implementation have any interactions between files. An API that reflects this seems more natural. e.g. it naturally provides operations like "are we tracking this file", and makes it natural to be able to e.g. lock at the per-file level. e.g.
> 
>   class WorkingSet {
>      shared_ptr<TranslationUnit> get(Path);
>      shared_ptr<TranslationUnit> getOrCreate(Path)
>    }
>    class TranslationUnit {
>      void update(ParseInputs);
>      void read(Callback);
>    }
> 
> This seems like it would make it easier to explain what the semantics of scheduleRemove are, too.
> 
> 2) The callbacks from individual methods seem more powerful than needed, and encourage deeper coupling. Basically, the inputs (changes) and outputs (diagnostics, reads) don't seem to want to interact with the same code. This suggests decoupling them: the changes are a sequence of input events, diagnostics are a sequence of output events, reads look much as they do now.
> 
> One benefit here is that the versioning relationship between inputs and outputs no longer show up in the function signatures (by coupling an input to a matching output). Expressing them as data makes it easier to tweak them.
> 
> 3) It's not spelled out how this interacts with drafts: whether text<->version is maintained here or externally, and what the contracts around versions are. There are no options offered, so I would guess that scheduleUpdate delivers new-version-or-nothing, and scheduleASTRead delivers... current-or-newer? current-or-nothing?
> 
> I think it would *probably* be clearer to have versions minted by the external DraftStore, that way we can decouple "we know about the contents of this file" from "we're building this file". e.g. we probably want wildly different policies for discarding resources of old versions, when "resources" = source code vs "resources" = ASTs and preambles.
> 
> 4) Scheduler (or anything it decomposes into) is important and isolated enough that it deserves its own header.
1. Given the nature of the LSP, the users of the interface will probably always call only a single function of `TranslationUnit`, so we won't win much in terms of the code clarity.
```
scheduleUpdate(File, Inputs) --> get(File).update(Inputs)
```
That adds some complexity to the interface, though. I'd opt for not doing that in the initial version.

2. One place where I find these callbacks useful are tests where we could wait for latest `addDocument` to complete. A more pressing concern is that the interface of `ClangdServer` does not allow to remove the callbacks (`addDocument` returns a future) and I would really love to keep the public interface of `ClangdServer` the same for the first iteration.

3. I would err on the side of saying that `scheduleASTRead` delivers current version. This gives correct results for the current callers of the interface. If we do current-or-newer I'd start with adding the versions to the interface, so that the callers of the API would have enough context to know whether the results correspond to the latest version or not. I'd really love to keep version out of the initial patch, happy to chat about them for follow-up patches.

4.  Totally agree. I planned to move it into a separate header after the changes land. Happy to do that with a follow-up patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42174