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

Duncan P. N. Exon Smith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 11 17:09:54 PST 2022


dexonsmith added a comment.

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

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

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

That'd be the usual incremental flow, I think. It also gives out-of-tree clients a good commit window to adopt the new API:

- Temporarily revert D121424 <https://reviews.llvm.org/D121424> on out-of-tree branch to allow merging `main`.
- Update out-of-tree code to use the new API.
- Reapply D121424 <https://reviews.llvm.org/D121424> to match `main` again.

This also has the major benefit of making post-commit review (e.g., reading `git log -u`) easier to manage.

Maybe it's not worth it here -- maybe it's a lot of work to make these separately-landable and maybe there aren't (known) external uses of `RedirectingFS` -- but naïvely, based on seeing three Phab reviews and having a better understanding now of what's in each, it seems like it might not be hard to keep them split?

(If there's some temporary regression to worry about, it's better to unify them, but otherwise separate commits would be preferred I think.)

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

I meant D121425 <https://reviews.llvm.org/D121425>, then D121424 <https://reviews.llvm.org/D121424> + D121426 <https://reviews.llvm.org/D121426>, because I assumed those last two were intrinsically linked, but maybe they aren't?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121424



More information about the llvm-commits mailing list