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

Wed Nov 27 04:23:34 PST 2019

jhenderson added a comment.

In D70769#1761517 <https://reviews.llvm.org/D70769#1761517>, @sammccall wrote:

> It's not just unit-tests, in D70222 <https://reviews.llvm.org/D70222> we will ultimately use FSes other than getRealFilesystem() in clangd.

I see, thank you.

> 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?

A default argument would be my preference there. It looks like we have some already.

================
Comment at: llvm/include/llvm/Support/CommandLine.h:1963
 /// \param [in] Tokenizer Tokenization strategy. Typically Unix or Windows.
+/// \param [in] FS VFS used for all file accesses when running the tool.
 /// \param [in,out] Argv Command line into which to expand response files.
----------------
Does `VFS` make sense here, given that it could be a real filesystem?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70769