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