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

Fri Mar 11 16:15:27 PST 2022

bnbarham added a comment.

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

> In this case:
>
> - I think there might be a way to re-sequence D121425 <https://reviews.llvm.org/D121425> ahead of this commit.
>   - The new API can be added and unit tested without deleting features from RedirectingFS.
>   - There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
> - Maybe this commit could be made runtime-configurable (`OverlayFS` constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426 <https://reviews.llvm.org/D121426>? Maybe that's too complicated; just pointing out the flexibility.
>   - Else, I'd suggest merging D121426 <https://reviews.llvm.org/D121426> with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.

Landing D121425 <https://reviews.llvm.org/D121425> before this one is definitely *possible*, but I'd really rather this change be atomic - neither really make sense on their own (IMO). The last option you listed said "merging D121426 <https://reviews.llvm.org/D121426> with this one", but that one is basically just API updates and a few test fixes for the new behavior. Did you mean with D121425 <https://reviews.llvm.org/D121425> or just all of them together?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424