keith accepted this revision.
keith added a comment.
This revision is now accepted and ready to land.
I'm definitely not a VFS expert, but I do think this is much nicer than trying
to do the other workarounds with multiple VFSs that you described, I left some
minor comments, I reviewed carefully but this logic was and is quite dense so
I'm heavily relying on the test coverage.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:575
+/// instead>
+/// 'redirecting-with': <string, one of 'fallthrough', 'fallback', or
+/// 'redirect-only', default='fallthrough'>
----------------
I think `redirecting-with` is fine, and I can't come up with something better
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1341
+ Iters.push_back(ExternalIter);
+ } else {
+ Iters.push_back(ExternalIter);
----------------
I know we can only get here if the redirection != redirectOnly, but I think it
might be more readable if we had an explicit else if here with fallthrough, and
then added an else with an unreachable to make it clear, and make sure if we
ever move stuff around we revisit this ordering
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1455-1456
+ return RedirectingFileSystem::RedirectKind::RedirectOnly;
+ }
+ return None;
+ }
----------------
Should this case error because the set value was invalid / a typo? Maybe this
bubbles up actually?
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1870
+ }
+ } else if (Key == "redirecting-with") {
+ if (auto Kind = parseRedirectKind(I.getValue())) {
----------------
Should we error in the case that both this key and `fallthrough` are present?
Maybe that's too strict for backwards compat, but right now I guess
`fallthrough` could overwrite this value depending on order?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117937/new/
https://reviews.llvm.org/D117937
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
