[PATCH] D70769: [Support] add vfs support for ExpandResponseFiles

[Sam McCall via Phabricator via cfe-commits]

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