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