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

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 22 13:53:48 PDT 2022


rnk added a comment.

This makes sense to me.

This probably affects nobody, but this reminds me of my first LLVM change: https://github.com/llvm/llvm-project/commit/fc8a2d5a8390952029e1c47a623e046b744f44d4



================
Comment at: llvm/include/llvm/Support/CommandLine.h:2098
+public:
+  ExpansionContext(StringSaver &S, TokenizerCallback T,
+                   llvm::vfs::FileSystem *FS = nullptr);
----------------
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.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:2099
+  ExpansionContext(StringSaver &S, TokenizerCallback T,
+                   llvm::vfs::FileSystem *FS = nullptr);
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132379



More information about the llvm-commits mailing list