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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
