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