[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed Oct 31 02:16:15 PDT 2018
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
Not needed anymore?
Comment at: include/lldb/Utility/FileSpec.h:423
- /// Returns a std::string with the directory and filename
+ /// Retuthe rns a std::string with the directory and filename
> 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") !=
+ EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/bar") !=
+ EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/baz") !=
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).
More information about the lldb-commits