[PATCH] D121424: [VFS] Revert RedirectingFileSystem to having a single responsibility

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D121424#3376840 <https://reviews.llvm.org/D121424#3376840>, @bnbarham wrote:

> My thinking was that the state after D121425 <https://reviews.llvm.org/D121425> + D121426 <https://reviews.llvm.org/D121426> would be such that the `redirecting-with` options are implemented in both `OverlayFS` and `RedirectingFS`, which I was worried would lead to some confusion if we end up needing to revert/looking at history later/etc. I'd also need to split the parsing changes out of this patch, move into D121425 <https://reviews.llvm.org/D121425>, and then add a bit extra to it to still set `redirecting-with` on `RedirectingFS`.
>
> Perhaps that isn't as bad as I was imagining though. I'm a little worried it's just Friday night and I'm not thinking this through clearly, so I'll give it the weekend and try it on Monday if I don't think of anything.

Doesn't sound too bad to me, but let me know what you think. In the first commit adding the new API you can add FIXME/TODOs to the old APIs indicating that the functionality is going away to make it clear what the plan is.

One major benefit is that if there is another client (besides clang), and you need to temporarily revert the "delete the old API patch", it's much less revert-churn that reverting the whole shebang (also, it doesn't block forward progress in clang).

>> add tests proving that A->B->C case no longer works
>
> Are you asking for a test that manually nests `RedirectingFileSystem` and checks that this case doesn't work? Or just the existing `TEST(VFSFromYAMLsTest, NotTransitive)` test in D121425 <https://reviews.llvm.org/D121425>? For `RedirectingFileSystem` all we really want to check is that `ExternalFS` isn't used as a fallback, which I believe is already tested in the various `VFSFromYAMLTest` cases. That actually brings me to something I was meaning to bring up...
>
> One thing that I haven't gotten to in these patches is to fix up the VFS tests in general - I only started laying the groundwork for them. We currently have a whole bunch of `VFSFromYAMLTest` cases that should probably be split up into testing `OverlayFS`, `RedirectingFS`, and then the actual `getVFSFromYAML(s)` API itself. My guess is that ended up getting mixed in together because `RedirectingFS` became the implementation of `-ivfsoverlay` and thus `getVFSFromYAML` *was* `RedirectingFileSystem`.
>
> I almost did that in this patch, but thought that it would make it more difficult to review (since other than the A -> B -> C chaining, there shouldn't be any functional changes from the `-ivfsoverlay` perspective). I would like to do that as a follow up though if that makes sense to you.

One historical reason is that RedirectingFileSystem wasn't previously exposed in a header. Would be great to get this cleaned up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424