[PATCH] D91317: Support: Add RedirectingFileSystem::create from simple list of redirections
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 8 15:03:30 PST 2020
dexonsmith added inline comments.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:735
+ /// Redirect each of the remapped files from first to second.
+ static std::unique_ptr<RedirectingFileSystem>
+ create(ArrayRef<std::pair<std::string, std::string>> RemappedFiles,
----------------
dexonsmith wrote:
> JDevlieghere wrote:
> > - Do you need to know that this is a `RedirectingFileSystem` or can it be a `FileSystem`?
> > - Is there a reason this should be a `unique_ptr` instead of an `IntrusiveRefCntPtr` (which is more prevalent across the VFS, although I'm not sure why)
> For my use case it doesn't matter what kind of `FileSystem` is returned; I figure this could be convenient for a future client. I would tend toward keeping it this type to match the other overload of `RedirectingFileSystem::create`, but happy to change it if you disagree.
>
> Regarding `unique_ptr`, I chose that as a sounder alternative to the raw pointer above; leaves options open to clients if they do want unique ownership, which seems nice to support since it's trivial. My use case will ultimately assign it to an `IntrusiveRefCntPtr` so if you disagree I can change it.
It does seem nicer to be consistent, so I just sent out https://reviews.llvm.org/D92888, which allows constructing an `IntrusiveRefCntPtr` from a moved `unique_ptr`, and https://reviews.llvm.org/D92890, which update existing APIs to use `unique_ptr`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91317/new/
https://reviews.llvm.org/D91317
More information about the cfe-commits
mailing list