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