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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

In D94844#2524854 <https://reviews.llvm.org/D94844#2524854>, @nathawes wrote:

> @JDevlieghere and @dexonsmith would you mind taking another look?
>
> I ended up changing the 'lookup' functions to drop the ExternalRedirect out param and supply that via a new 'LookupResult' return value that also gives the matched Entry. Hopefully that avoids any confusion over where it's computed vs used. It also served as a good home for the duplicated code and the logic in the old SetExternalRedirect closure, as Jonas had mentioned. While adding the extra directory iteration tests Duncan suggested I noticed I wasn't respecting the 'use-external-names' setting in the return items, so I've also added a new directory iterator implementation (RedirectingFSDirRemapIterImpl). That one's used to wrap the iterator the external file system gives for the directory being mapped to, and updates the paths in the items it returns to refer to the virtual directory's path instead, before passing them along.

I like the new approach, it adds an indirection but nicely abstracts how we get the `ExternalRedirect`. I've left an inline comment about a copy I think we can avoid, but other than that this LGTM.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:748
+  bool shouldFallBackToExternalFS(std::error_code EC,
+                                  Optional<LookupResult> Result = None) const;
 
----------------
While I prefer `Optional` over a pointer, this would now incur a copy, so I think it's better to pass a `LookupResult*` here, or maybe even a `RemapEntry*`. 


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

https://reviews.llvm.org/D94844