[PATCH] D91317: Support: Add RedirectingFileSystem::create from simple list of redirections

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

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