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

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

Thanks for taking a look at that @keith, I appreciate it :)! I'll make those changes today hopefully.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:575
+///                   instead>
+///   'redirecting-with': <string, one of 'fallthrough', 'fallback', or
+///                        'redirect-only', default='fallthrough'>
----------------
keith wrote:
> I think `redirecting-with` is fine, and I can't come up with something better
Thanks. Do you know if this format is documented anywhere that I would need to update?


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1341
+    Iters.push_back(ExternalIter);
+  } else {
+    Iters.push_back(ExternalIter);
----------------
keith wrote:
> 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
Ah yep, I really should have done that in the first place. Thanks!


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1455-1456
+      return RedirectingFileSystem::RedirectKind::RedirectOnly;
+    }
+    return None;
+  }
----------------
keith wrote:
> Should this case error because the set value was invalid / a typo? Maybe this bubbles up actually?
It ends up returning `false` down in `parse` but I need to add a call to `error` to actually describe the error.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1870
+        }
+      } else if (Key == "redirecting-with") {
+        if (auto Kind = parseRedirectKind(I.getValue())) {
----------------
keith wrote:
> 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?
I intentionally allowed both for backwards compatibility, though I'm not sure if that is a real concern after more thinking about it - you'd have to use the new key for it to matter, in which case we don't need to worry about backwards compatibility. So yes, I think we should error if both are present.

I need to add tests for all these error cases as well (both keys present and passing an incorrect value).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117937