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