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

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
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;
----------------
haowei wrote:
> bnbarham wrote:
> > `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.
> Thanks for pointing that out. I need to consider the windows forward slash case.
> 
> There is a bit in consistency in the VFS implementation. for Windows. we can use forward slashes('/') on Windows. Most Windows API/Syscalls accept it. But some APIs may not work if you mix '/' with '\' in the path on Windows. What RedirectFileSystem::makeAbsolute is trying to do is, it first determine the slashes used in the WorkDir(either from process working directory or from command line flags, or OverlayDir), in which it can be a forward slash on Windows. Then it uses the same separator when appending the relative directory.  Using sys::fs::make_absolute will break that. 
> 
> The reason that I use makeAbsolute here is that the OverlayDir will likely use forward slash on Windows if people uses CMake options LINK_FLAG="/vfsoverlay:C:/path/to/overlay.xml". In that case, sys::fs::make_absolute will generate some paths like C:/path/to/foo\\bar, which may cause issues.
> 
> I will rewrite this to consider the forward slashes.
This was also discussed in D87732 which may provide some additional context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473