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

[Haowei Wu via Phabricator via cfe-commits]

haowei marked 2 inline comments as done.
haowei added inline comments.


================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:1010
 
+  std::error_code makeAbsolute(StringRef WorkingDir,
+                               SmallVectorImpl<char> &Path) const;
----------------
bnbarham wrote:
> Should be private IMO
Moved it to private section and added comments


================
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,
----------------
bnbarham wrote:
> 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.
No, it is not needed for now. As the OverlayDir is always abs when it was set. The normal makeAbsolute function checks "Path" param but not checking the returned WorkDir, which is OK because the return value from getCurrentWorkingDirectory will be abs or empty.

It is more to be future proof if someone decide to use this function for other purpose and putting a non-abs path in WorkingDir field will causes unexpected behaviors.

I also need to exclude empty WorkingDir though because in unit tests, the WorkingDir is always empty.



================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1913
+                 RedirectingFileSystem::RootRelativeKind::OverlayDir) {
+        SmallString<256> FullPath = FS->getOverlayFileDir();
+        assert(!FullPath.empty() && "Overlay file directory must exist");
----------------
bnbarham wrote:
> 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.
I merged these 2 else block.
I have to use sys::fs::make_absolute function for now as using FS's CWD will break a few tests. It is better to address these test in a separate patch.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473