[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/include/llvm/Support/VirtualFileSystem.h:1010
 
+  std::error_code makeAbsolute(StringRef WorkingDir,
+                               SmallVectorImpl<char> &Path) const;
----------------
Should be private IMO


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1369
   // append Path ourselves.
+  if (!sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+      !sys::path::is_absolute(WorkingDir,
----------------
Did you find this was needed? It's already checked by the normal `makeAbsolute` (which checks this already) and the only other caller is when we're using the overlay directory path, which should always be absolute.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1913
+                 RedirectingFileSystem::RootRelativeKind::OverlayDir) {
+        SmallString<256> FullPath = FS->getOverlayFileDir();
+        assert(!FullPath.empty() && "Overlay file directory must exist");
----------------
This is unused now. Maybe we should just merge the `else if` and `else` branches and just grab either `getOverlayFileDir` or `getCurrentWorkingDirectory` depending on `RootRelative`. They should otherwise be identical.


================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1875
+
+  IntrusiveRefCntPtr<vfs::FileSystem> FSOverlayRelative =
+      getFromYAMLString("{\n"
----------------
Just override the previous `FS`/`S` IMO - that way we avoid the accidental `FS->status` a few lines down 😅


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473