[PATCH] D117937: [VFS] Add a "redirecting-with" field to overlays

[Keith Smiley via Phabricator via cfe-commits]

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