[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

[Pavel Labath via Phabricator via lldb-commits]

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