[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support to FileSpec convenience methods.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I also think it we should try to keep FileSpec in the Utility module. We should be able to do that by deleting the "utility" functions in the FileSpec class, and updating everything to go through the FileSystem class.

After the VFS introduction, something like `file_spec.Exists()` will not even be well-defined anyway, as one will have to specify which file system are we checking the existence in. So, it would be more correct to say `file_spec.Exists(my_vfs)`, at which point it's not too much of a stretch to change this into `my_vfs.Exists(file_spec)`.

(This is the same argument I was giving earlier, since even before VFS we kinda were dealing with multiple file systems (host, remote device). In fact, after this it might be a nice cleanup to change the Platform class from vending a bunch of FS-like methods to (FileExists, Unlink, CreateSymlink, ...), to have a single `getRemoteFS` method returning a VFS which does the right thing "under the hood").

tl;dr: I'd propose to (in two or more patches):

- replace FileSpec "utility" methods with equivalent FileSystem functionality
- do the necessary FileSystem changes to support VFS



================
Comment at: source/Host/common/FileSystem.cpp:24-29
+  static FileSystem *g_fs;
+  static llvm::once_flag g_once_flag;
+  llvm::call_once(g_once_flag, []() {
+    if (g_fs == nullptr)
+      g_fs = new FileSystem();
+  });
----------------
replace with `static FileSystem *g_fs = new FileSystem();`


================
Comment at: source/Host/common/FileSystem.cpp:33-35
+void FileSystem::ChangeFileSystem(IntrusiveRefCntPtr<vfs::FileSystem> fs) {
+  m_fs = fs;
+}
----------------
I'm not sure what's your use case here, but would it make more sense instead of modifying the singleton object to have more that one FileSystem instance floating around? It could be local to each `Debugger` (?) object and everyone would fetch it from there when needed.


https://reviews.llvm.org/D53532