dexonsmith added reviewers: benlangmuir, bnbarham, jansvoboda11. dexonsmith added a comment.
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. 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
