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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Host/common/FileSystem.cpp:48
 
-void FileSystem::Initialize() {
+llvm::Error FileSystem::Initialize() {
   lldbassert(!InstanceImpl() && "Already initialized.");
----------------
Would it be possible to have the overloads which always succeed keep returning void? Here, you've had to add failure checks in SystemInitializerCommon, which are essentially dead code, because the error path will never be taken.


================
Comment at: source/Host/common/FileSystem.cpp:443
+
+  // If VFS mapped we know the underlying FS is a RedirectingFileSystem.
+  ErrorOr<vfs::Entry *> E =
----------------
JDevlieghere wrote:
> 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?
It's not a big thing, but the advantage would be that it would be possible to locally verify that the up/down-casting is correct just by looking at the FileSystem class. This way you have to peek into the implementation of getVFSFromYAML to see that it indeed always returns the right type. Also, if anyone later gets the idea of changing the type returned by this function, he'll be more likely to thing about possible breakages down the line.


================
Comment at: source/Initialization/SystemInitializerCommon.cpp:82
+  if (repro::Loader *loader = r.GetLoader()) {
+    if (auto info = loader->GetProviderInfo("files")) {
+      FileSpec vfs_mapping(loader->GetRoot());
----------------
JDevlieghere wrote:
> 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. 
Ok, I see what you mean. That should still be possible to achieve even with stricter type checking. You'd just need to have a separate type for the "info", which can be shared by the replay and capture classes.


================
Comment at: source/Initialization/SystemInitializerCommon.cpp:87
+    } else {
+      // No file provider, continue with the real filesystem.
+      FileSystem::Initialize();
----------------
JDevlieghere wrote:
> 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.  
Ok, makes sense.


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

https://reviews.llvm.org/D54617