[PATCH] D132379: [Support] Class for response file expansion

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:2098
+public:
+  ExpansionContext(StringSaver &S, TokenizerCallback T,
+                   llvm::vfs::FileSystem *FS = nullptr);
----------------
rnk wrote:
> StringSaver is a stateless class that wraps a BumpPtrAllocator. To simplify the call site, I suggest making this parameter into a `BumpPtrAllocator &`, and construct a field StringSaver from the allocator.
Indeed in many cases StringSaver in caller can be omitted if the parameter is BumpPtrAllocator.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:2099
+  ExpansionContext(StringSaver &S, TokenizerCallback T,
+                   llvm::vfs::FileSystem *FS = nullptr);
+
----------------
rnk wrote:
> It feels like you are using the builder pattern to handle setting uncommon options. If that's what we're doing, should we commit completely and add a `.setVFS()` helper and remove the optional parameter?
You are right. Changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132379