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