sammccall added a comment. In D70769#1761494 <https://reviews.llvm.org/D70769#1761494>, @jhenderson wrote:
> In D70769#1761428 <https://reviews.llvm.org/D70769#1761428>, @lh123 wrote: > > > In D70769#1761394 <https://reviews.llvm.org/D70769#1761394>, @jhenderson > > wrote: > > > > > Are there any instances where we DON'T want to get the real file system? > > > If not, could the `*llvm::vfs::getRealFileSystem()` call be put inside > > > `cl::ExpandResponseFiles`? > > > > > > This patch is part of D70222 <https://reviews.llvm.org/D70222>. > > > > This is for using InMemoryFileSystem in unit tests. > > > Okay, that makes sense. Perhaps we should introduce a new overload of > `ExpandResponseFiles` that allows specifying a different file system then, > and having the current version call into that one, specifying the > getRealFileSystem call? I'm not overly keen on having yet another apparently > boiler-plate piece of functionality required in every single call of > `ExpandResponseFiles`. It's not just unit-tests, in D70222 <https://reviews.llvm.org/D70222> we will ultimately use FSes other than getRealFilesystem() in clangd. Having non-VFS-clean versions of functions around that silently use the real FS is a bit of a scourge for users that need to be VFS-clean, honestly. Parsing argv is exactly the sort of place where FS access isn't obvious and exposing a function that doesn't accept a VFS encourages callers to miss this aspect. At the same time this is mostly called from a bunch of drivers that (presumably) don't need VFS support. Maybe allowing nullptr = real FS, or a default argument, would be an OK compromise? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70769/new/ https://reviews.llvm.org/D70769 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
