labath added a comment.
I don't have any major issues with this patch. The thing I'd like to see though
is one test which uses a path with `..` components and symlinks. If nothing
else, it would serve as a good documentation for how these are supposed to be
handled.
================
Comment at: include/lldb/Utility/Reproducer.h:91-92
+
+const char *FileInfo::name = "files";
+const char *FileInfo::file = "files.yaml";
+
----------------
JDevlieghere wrote:
> labath wrote:
> > Are you sure this can be in a header? I would expect this to give you
> > multiply-defined symbols errors.
> You are correct, should be in the implementation.
It looks like these are still in the header. :)
================
Comment at: source/Utility/FileCollector.cpp:27-33
+ // Change path to all upper case and ask for its real path, if the latter
+ // exists and is equal to path, it's not case sensitive. Default to case
+ // sensitive in the absence of real_path, since this is the YAMLVFSWriter
+ // default.
+ upper_dest = path.upper();
+ if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest))
+ return false;
----------------
JDevlieghere wrote:
> davide wrote:
> > should this be a function in FS to check the case-sensitiveness?
> As Host depends on Utility, we cannot depend on the filesystem class here.
FWIW, I think it makes sense for FileCollector to *not* use the FileSystem
class, as FileCollector is pretty much an implementation detail of the
FileSystem.
================
Comment at: unittests/Utility/FileCollectorTest.cpp:31-67
+TEST(FileCollectorTest, AddFile) {
+ FileSpec root("/root", FileSpec::Style::posix);
+ TestingFileCollector file_collector(root);
+
+ file_collector.AddFile("/path/to/a");
+ file_collector.AddFile("/path/to/b");
+ file_collector.AddFile(FileSpec("/path/to/c", FileSpec::Style::posix));
----------------
I have some doubts about whether this will correctly run on windows (mixing of
posix and native paths sounds worrying). It might be better to just have this
test always work with host paths (as these class should always deal with files
on the host system anyway).
================
Comment at: unittests/Utility/FileCollectorTest.cpp:71-72
+ std::error_code ec;
+ // Create a unique directory. We cannot depend on lldb's filesystem for this
+ // because it depends on Utility.
+ SmallString<128> collector_root;
----------------
For testing, you can theoretically pull in other modules if they are needed
(but I don't think that's the case here).
================
Comment at: unittests/Utility/FileCollectorTest.cpp:78-80
+ SmallString<128> file_a;
+ ec = sys::fs::createTemporaryFile("file", "a", file_a);
+ EXPECT_FALSE(ec);
----------------
If you made these into `ScopedFile` classes like the VFS tests do then besides
making this shorter, you'd also make this test self-cleaning in most of the
cases (I fear this can start choking the buildbots after a couple of thousand
runs...)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54617/new/
https://reviews.llvm.org/D54617
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
