[PATCH] D117730: [DNM][VFS] Do not overwrite the path when nesting RedirectingFileSystems

[Ben Barham via Phabricator via llvm-commits]

bnbarham added inline comments.


================
Comment at: clang/test/VFS/directory.c:2
 // RUN: rm -rf %t
-// RUN: mkdir -p %t/Underlying
-// RUN: mkdir -p %t/Overlay
-// RUN: mkdir -p %t/Middle
-// RUN: echo '// B.h in Underlying' > %t/Underlying/B.h
-// RUN: echo '#ifdef NESTED' >> %t/Underlying/B.h
-// RUN: echo '#include "C.h"' >> %t/Underlying/B.h
-// RUN: echo '#endif' >> %t/Underlying/B.h
-// RUN: echo '// C.h in Underlying' > %t/Underlying/C.h
-// RUN: echo '// C.h in Middle' > %t/Middle/C.h
-// RUN: echo '// C.h in Overlay' > %t/Overlay/C.h
-
-// 1) Underlying -> Overlay (C.h found, B.h falling back to Underlying)
-// RUN: sed -e "s at INPUT_DIR@%{/t:regex_replacement}/Overlay at g" -e "s at OUT_DIR@%{/t:regex_replacement}/Underlying at g" %S/Inputs/vfsoverlay-directory.yaml > %t/vfs.yaml
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -fsyntax-only -DNESTED -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-// RUN: sed -e "s at INPUT_DIR@Overlay at g" -e "s at OUT_DIR@%{/t:regex_replacement}/Underlying at g" %S/Inputs/vfsoverlay-directory-relative.yaml > %t/vfs-relative.yaml
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs-relative.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-
-// DIRECT: {{^}}// B.h in Underlying
-// DIRECT: {{^}}// C.h in Overlay
-
-// 2) Underlying -> Middle -> Overlay (C.h found, B.h falling back to Underlying)
-// RUN: sed -e "s at INPUT_DIR@%{/t:regex_replacement}/Overlay at g" -e "s at OUT_DIR@%{/t:regex_replacement}/Middle at g" %S/Inputs/vfsoverlay-directory.yaml > %t/vfs.yaml
-// RUN: sed -e "s at INPUT_DIR@%{/t:regex_replacement}/Middle at g" -e "s at OUT_DIR@%{/t:regex_replacement}/Underlying at g" %S/Inputs/vfsoverlay-directory.yaml > %t/vfs2.yaml
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -DNESTED -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=DIRECT %s
-
-// Same as direct above
-
-// 3) Underlying -> Middle -> Overlay (C.h falling back to Middle, B.h falling back to Underlying)
-// RUN: rm -f %t/Overlay/C.h
-// RUN: %clang_cc1 -Werror -I %t/Underlying -ivfsoverlay %t/vfs.yaml -ivfsoverlay %t/vfs2.yaml -fsyntax-only -E -C %s 2>&1 | FileCheck --check-prefix=FALLBACK %s
-
-// FALLBACK: {{^}}// B.h in Underlying
-// FALLBACK: {{^}}// C.h in Middle
-
-// 3) Underlying -> Middle -> Overlay (C.h falling back to Underlying, B.h falling back to Underlying)
+// RUN: split-file %s %t
+
----------------
dexonsmith wrote:
> This makes it hard to read what has changed. Probably better to commit an NFC patch (updating the testcase with no behaviour change) and then rebase this patch on top.
Yep, to be clear this is very much a WIP PR - I mostly put it up to get some feedback. I expect there to be a fair bit to clean up.

The only thing actually *changed* here is the addition of checking the path that was written out as well. The rest was to help me understand what was actually going on originally, I'll probably just revert and just add the path if we do end up wanting this (unless people prefer split-file).


================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:2714-2716
+  // Test the interaction of two overlays where one maps back to the other,
+  // ie. `a` -> `b` and then `b` -> `a`. This should always use `a` if it
+  // exists and fallback to `b` otherwise.
----------------
dexonsmith wrote:
> Remapping in two directions seems like an extra layer of complexity; is that the case where this came up, or was it something more straightforward?
It shows up in A -> B -> C (as in the non-unit test) so this one could test that instead. I did this when originally reproducing the issue and only figured out after that it was just due to nesting in general.

The problem is nested RedirectingFileSystems, where the outermost now overwrites the paths written by the nested FS's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117730