[PATCH] D94844: [VFS] Add support to RedirectingFileSystem for mapping a virtual directory to one in the external FS.

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

I don't see a test for a case where the mapped directory already exists in the external FS, something like:

  # external fs
  /d1/f1
  /d2/f2
  
  # redirect fs from yaml (fallthrough: true)
  /d1 -> /d2

Unless I just missed it, can you add one?

My intuition is we'd want to overlay the directory contents, not replace them:

  # overlayed contents (my intuition for fallthrough: true)
  /d1/f1
  /d1/f2
  /d2/f2
  
  # replaced contents (I think unexpected)
  /d1/f2
  /d2/f2

If you agree, it might be good to test that the recursive directory iterator does the right thing.

Might also be valuable to test the non-fallthrough case, which I assume should give this view:

  # fallthrough: false
  /d1/f2

@JDevlieghere, I'm hoping you can help review the details of the code here, as I don't know the iteration code well. Also, are these API changes okay for LLDB (I seem to remember LLDB subclassing this...)?



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:608-610
+  /// A file or directory in the vfs that is mapped to a file or directory in
+  /// the external filesystem.
+  class RemapEntry : public Entry {
----------------
Is it easy to move this declaration after `DirectoryEntry`? If so, I think that'd reduce the diff a bit, and make it more clear what parts of this were being used verbatim from the old `FileEntry`. Up to you though.


================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:776-778
   /// Looks up \p Path in \c Roots.
-  ErrorOr<Entry *> lookupPath(const Twine &Path) const;
+  ErrorOr<Entry *> lookupPath(const Twine &Path,
+                              SmallVectorImpl<char> &ExternalRedirect) const;
----------------
It's not entirely clear what the extra parameters do / when they're filled in / etc; can you add a bit more documentation?

Also, I see that the public caller ignores the `ExternalRedirect` parameter. I wonder if this should be renamed to `lookupPathWithName` or `lookupPathImpl`, leaving behind a `lookupPath` wrapper that creates the ignored buffer for the public interface... no strong opinion though.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1831
+  // directory's external contents path plus any remaining path components.
+  auto SetExtRedirect = [&](RedirectingFileSystem::Entry *E) {
+    auto *DRE = dyn_cast<RedirectingFileSystem::DirectoryRemapEntry>(From);
----------------
Could this return `E`, and then the callers `return SetExtRedirect(...)`? Not sure if it'd actually be cleaner, up to you.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2039
     }
-    return;
+  } else if (Kind == RedirectingFileSystem::EK_RemapDirectory) {
+    auto *DR = dyn_cast<RedirectingFileSystem::DirectoryRemapEntry>(SrcE);
----------------
Nit: please keep the early return.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2047
+        YAMLVFSEntry(VPath.c_str(), DR->getExternalContentsPath()));
+  } else {
+    assert(Kind == RedirectingFileSystem::EK_File && "Must be a EK_File");
----------------
Nit: please use an early return here to avoid unnecessary indentation (I think the rest of this diff goes away...).


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:2068-2070
+  SmallString<256> ExternalRedirect;
+  ErrorOr<RedirectingFileSystem::Entry *> RootE =
+      VFS->lookupPath("/", ExternalRedirect);
----------------
This seems a bit unfortunate to have to change, but not a big deal...


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

https://reviews.llvm.org/D94844