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

Fri Mar 11 22:31:59 PST 2022

bnbarham added a comment.

In D121424#3376440 <https://reviews.llvm.org/D121424#3376440>, @dexonsmith wrote:

> Can you explain more why you don't think it's a good idea in this case?
>
> For complicated changes, it's not uncommon to add the new API (with tests for it), and then do adoption later.
>
> Actually, I wonder if you could even just land these as three separate commits by doing D121424 <https://reviews.llvm.org/D121424> last:
>
> - D121425 <https://reviews.llvm.org/D121425>: add new API (causes any created RedirectingFS to be created with these options turned off)
> - D121426 <https://reviews.llvm.org/D121426>: change clang to use it
> - D121424 <https://reviews.llvm.org/D121424>: remove now-unneeded RedirectingFS code and add tests proving that `A->B->C` case no longer works

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.

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424