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