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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
