[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via llvm-commits]

bnbarham added inline comments.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1922
+          EC = FS->makeAbsolute(FullPath, Name);
+          Name = canonicalize(Name);
+        } else {
----------------
Why the canonicalization?


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1933-1935
         path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
                          ? sys::path::Style::posix
                          : sys::path::Style::windows_backslash;
----------------
`windows_slash` is going to be `windows_backslash` here. I assume this was fine since `sys::fs::make_absolute(Name)` would always return the native style, but that isn't the case now. It's also unfortunate we calculate this again when `makeAbsolute` only just did it.


================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32
----------------
`separator` -> `separator`

As per my comment above, this is only true because we're either using posix or windows_backslash. I'm not sure what we really want here, should we always convert to native? Any thoughts @dexonsmith?

FWIW the non-windows case is identical to the above block. It'd be interesting to test `/` and `\` on both filesystems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473