[PATCH] D80900: [clangd] Use different FS in PreambleThread

Wed Jun 3 07:06:09 PDT 2020

sammccall added a comment.

In D80900#2069566 <https://reviews.llvm.org/D80900#2069566>, @kadircet wrote:

> In D80900#2066327 <https://reviews.llvm.org/D80900#2066327>, @sammccall wrote:
>
> > TL;DR: I think there are three viable paths that we should consider:
> >  A) minimal change: put the FSProvider in ParseInputs
> >  B) this patch, but with more documentation and safety
> >  C) fight to clean up VFS multithreading semantics
> >
> > Assuming we don't want to block on C, I'm not sure whether A or B is conceptually better. But A is a smaller change and brings side benefits, so wondering if you see advantages in B.
>
>
> I didn't want to go for A) as I was afraid of satisfying the contract on `FileSystemProvider::getFileSystem`
>
>   /// Context::current() will be the context passed to the clang entrypoint,
>   /// such as addDocument(), and will also be propagated to result callbacks.
>   
>
> As this patch keep the `FileSystemProvider` inside TUScheduler.cpp and we need to ensure context is always the one we received in the entrypoint. Whereas `ParseInputs` is widely
>  passed around, and ensuring that assumption holds might be hard.

That makes sense. However I don't think that contract is conceptually that great, having the ideas of "threadsafe FS" and "maybe-context-aware FS" coupled together is... weird. The benefit is that it's an easier contract to satisfy.

It's already the case that the context could/should be plumbed through to the actual FS operations and other extension points. Things like tracing rely on it. I think saying "we always plumb context through to everything" is a better contract even if it's a bit stronger than we need and harder to prove satisfied. This frees up FileSystemProvider to be the conceptually-simple ThreadsafeFS that could be lifted from clangd/support to llvm/support if we want.

WDYT?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80900/new/

https://reviews.llvm.org/D80900