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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere planned changes to this revision.
JDevlieghere added a comment.

I'll add a more specific test for the collector.



================
Comment at: source/Host/common/FileSystem.cpp:443
+
+  // If VFS mapped we know the underlying FS is a RedirectingFileSystem.
+  ErrorOr<vfs::Entry *> E =
----------------
labath wrote:
> What are the chances of making this slightly more official by making `getVFSFromYAML` return a `IntrusiveRefCntPtr<RedirectingFileSystem>` instead of a generic pointer?
I'm not sure I understand what the benefit would be, we'd still store it as a generic FileSystem in the member variable?


================
Comment at: source/Initialization/SystemInitializerCommon.cpp:82
+  if (repro::Loader *loader = r.GetLoader()) {
+    if (auto info = loader->GetProviderInfo("files")) {
+      FileSpec vfs_mapping(loader->GetRoot());
----------------
labath wrote:
> Would it be possible to avoid magic strings here, and use something like `GetProviderInfo<FileProvider>()` instead?
My idea was to decouple the provider (used only for capture) and the info needed for replay, but I think we can improve things by merging them into one. I don't think I've run into a situation yet where we don't have access to the provider during replay. 


================
Comment at: source/Initialization/SystemInitializerCommon.cpp:87
+    } else {
+      // No file provider, continue with the real filesystem.
+      FileSystem::Initialize();
----------------
labath wrote:
> When can this happen? Should this be an error or maybe even an assert?
Yes, reproducers should still work with "some" functionality disabled. The infrastructure is not there yet, but it would be nice to test pieces individually.  


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

https://reviews.llvm.org/D54617