[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Now that I know your use case (I forgot that you're working on the replay now, and the mention of VFS threw me off in a completely different direction), I agree that having a single global filesystem is fine. As for the increased verbosity, I wonder if we should make the fs accessor a free function instead of a member (so that you write something like `hostFS().Symlink(..)` instead of `FileSystem::instance().Symlink(...)`.

(Theoretically, we might even make the VFS instance a global variable, and then we could keep the current pattern of accessing stuff via static methods, but I like the fact that this VFS thingy could then be reused for working with files on remote devices. Also, you may need a working FS instance in the guts of the replay code, to e.g., read the replay data.)



================
Comment at: include/lldb/Host/FileSystem.h:35
+
+  void ChangeFileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs);
+
----------------
I am wondering if we could make this work without exposing the ability to change the FS implementation mid-flight to everyone. How are you planning to initialize the record/replay? Would it be possible to pass in the VFS instance via some kind of an `Initialize` function (and have it be immutable from that point on)?


================
Comment at: include/lldb/Utility/FileSpec.h:423
   /// @return
-  ///     Returns a std::string with the directory and filename
+  ///     Retuthe rns a std::string with the directory and filename
   ///     concatenated.
----------------
accidental change?


https://reviews.llvm.org/D53532