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

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

Some comments. Thanks!



================
Comment at: include/lldb/Utility/FileCollector.h:5-7
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
You might want to update the license.


================
Comment at: include/lldb/Utility/FileCollector.h:50-54
+  std::mutex m_mutex;
+  FileSpec m_root;
+  llvm::StringSet<> m_seen;
+  llvm::vfs::YAMLVFSWriter m_vfs_writer;
+  llvm::StringMap<std::string> m_symlink_map;
----------------
Can we add comments to these member variables? Some are not entirely obvious to me, e.g. what's `m_root`? 
What's `m_mutex` is supposed to protect . I think we should document these invariants.


================
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;
----------------
should this be a function in FS to check the case-sensitiveness?


================
Comment at: source/Utility/FileCollector.cpp:84-87
+  // Canonicalize the source path by removing "..", "." components.
+  SmallString<256> virtual_path = absolute_src;
+  sys::path::remove_dots(virtual_path, /*remove_dot_dot=*/true);
+
----------------
Ditto (I thought there was one to canonicalize the source path?)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54617/new/

https://reviews.llvm.org/D54617