[PATCH] D124816: [Tooling] use different FileManager for each CWD

[Shi Chen via Phabricator via cfe-commits]

Kale added a comment.

In D124816#3494243 <https://reviews.llvm.org/D124816#3494243>, @dexonsmith wrote:

> Two high-level comments:
>
> - Making these functions `virtual` makes it harder to reason about possible implementations, but I don't think that's necessary for what you're doing here. Why not expose `ToolFileManager` as a container of filemanagers which ToolInvocation takes by parameter, then calls `getOrCreateFileManager()` on at time of need passing relevant arguments (e.g., CWD)?
>   - This could also create/manage the relevant VFS instance, to which the FileManager is fundamentally tied. See also below.
> - I don't think this goes far enough. The FileManager keeps state for any accessed file, which can be wrong any time the VFS changes at all, not just if the `CWD` is different. E.g., different `-ivfsoverlay` options are passed, or someone is reading from an `InMemoryFileSystem` vs. from disk.
>   - AFAICT, every call to `FileManager::setVirtualFileSystem()` is fundamentally unsound unless the new FS is equivalent to the old one or the filemanager hasn't been used yet.

Thanks for your careful and useful suggestions. With your explanation, I realized that the change of VFS of course should be considered as well. I'll revise this patch and tries to cope with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124816