haowei added inline comments.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1922
+ EC = FS->makeAbsolute(FullPath, Name);
+ Name = canonicalize(Name);
+ } else {
----------------
bnbarham wrote:
> Why the canonicalization?
If the Name here somehow has something like //foo/./../bar, it will cause match
failures. Canonicalize function removes these dots.
The setOverlayFileDir does not canonicalize the dir so this is quite common to
see.
================
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;
----------------
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.
================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+ // with native path seperator, regardless of the actual path seperator
+ // used in YAMLFilePath field.
+#ifndef _WIN32
----------------
rnk wrote:
> dexonsmith wrote:
> > bnbarham wrote:
> > > `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.
> > I think we might need a Windows person to help answer; @rnk, can you
> > suggest someone to help answer @bnbarham's question?
> I can try suggesting @hans, @compnerd, or @Bigcheese
The non-windows case enable the overlay-relative option. The above block
didn't. And that is where I discover the issue with overlay-relative always
uses native separator instead of trying to match OverlayDir.
The best approach for root-relative and overlay-relative option would probably
be matching the style of OverlayDir/WorkDir and unify the path separator if
they are mixed in a path.
But this change will likely affects a few tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137473/new/
https://reviews.llvm.org/D137473
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
