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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Thanks for bearing with me. I am very happy with how this turned out in the end. I have more suggestion on how to clean up the initialization, which you can incorporate if you like it, but I don't want to hold up the patch over that.



================
Comment at: include/lldb/Host/FileSystem.h:21
 
+#include <mutex>
 #include <stdint.h>
----------------
Not needed anymore?


================
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.
----------------
labath wrote:
> accidental change?
It seems this is still here... :(


================
Comment at: source/Host/common/FileSystem.cpp:32
+  FileSystem &instance = Instance();
+  instance.m_initialized = true;
+}
----------------
Instead of manual `m_initialized` management, what would you say to this:
- have a private `InstanceImpl` (or whatever) function, which returns an `Optional<FileSystem> &`
- `Initialize` does `InstanceImpl().emplace(VFS);` (it can check that the value is `None` beforehand if you want, but I am not worried about people accidentally calling `Initialize` twice).
- `Terminate` does `InstanceImpl().reset();`
- `Instance` does `return *InstanceImpl();`

I think this would be a bit cleaner as it would allow is to specify the VFS directly during construction. We would avoid having to construct a `FileSystem` object with a dummy VFS, only to replace it with another one later. And also you wouldn't need to subclass FileSystem in the unit tests just to change the implementation.


================
Comment at: unittests/Host/FileSystemTest.cpp:302-310
+  EXPECT_EQ(visited.size(), (size_t)4);
+  EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/foo") !=
+              visited.end());
+  EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/bar") !=
+              visited.end());
+  EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/baz") !=
+              visited.end());
----------------
You can write this more concisely (and with better error messages) as `EXPECT_THAT(visited, testing::UnorderedElementsAre("/foo", "/bar", "/baz", "/qux"));` (you need to include `gmock.h` for that to work).


https://reviews.llvm.org/D53532