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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Overall this looks good. I wonder if abstracting the ExternalRedirect as a small wrapper class around a SmallString would help. There's a few operations that are repeated, like the example below, and it could also house the logic that's currently in the lambda.

  if (ExternalRedirect.empty()) {
    assert(RE->getKind() == RedirectingFileSystem::EK_File);
    ExternalRedirect = RE->getExternalContentsPath();
  }

One thing I found tricky during the review was differentiating between places where the ExternalRedirect was computed and uses. Looking at the signatures the `lookupPath` functions are computing it and `status` is consuming it, although the latter then changes the StringRef (which I guess shouldn't persist?). I don't know if the wrapper could alleviate this as well.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:564
 ///
+/// Re-mapped directories are represented as
+/// /// \verbatim
----------------
I think we should expand on this a bit more to contrast a directory-remapped directory vs a file-remapped directory (for a lack of better term). Specifically, an empty file-remapped directory could be confusing, as it behaves quite differently but looks pretty similar in YAML. 

```
{
  'type': 'file',
  'name': <string>,
  'external-contents': <path to external file>
}
```

```
{
  'type': 'directory',
  'name': <string>,
  'contents': []
}
```

Looking at the two, I wonder if there would be any benefit to changing the `type` in the yaml to match the `NameKind`.





================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:624
+
+    /// whether to use the external path as the name for this file or directory.
+    bool useExternalName(bool GlobalUseExternalName) const {
----------------
nit: s/whether/Whether/


================
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;
----------------
dexonsmith wrote:
> 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.
+1.


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

https://reviews.llvm.org/D94844