rnk added inline comments.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078
+std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char>
&Path) const {
+ if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
+ llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
+ return {};
----------------
I think Posix-style paths are considered absolute, even on Windows. The
opposite isn't true, a path with a drive letter is considered to be relative if
the default path style is Posix. But, I don't think that matters. We only end
up in this mixed path style situation on Windows.
Does this change end up being necessary? I would expect the existing
implementation of makeAbsolute to do the right thing on Windows as is. I think
the other change seems to be what matters.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
- if (IsRootEntry && !sys::path::is_absolute(Name)) {
- assert(NameValueNode && "Name presence should be checked earlier");
----------------
Is there a way to unit test this? I see some existing tests in
llvm/unittests/Support/VirtualFileSystemTest.cpp.
I looked at the yaml files in the VFS tests this fixes, and I see entries like
this:
{ 'name': '/tests', 'type': 'directory', ... },
{ 'name': '/indirect-vfs-root-files', 'type': 'directory', ... },
{ 'name': 'C:/src/llvm-project/clang/test/VFS/Inputs/Broken.framework',
'type': 'directory', ... },
{ 'name':
'C:/src/llvm-project/build/tools/clang/test/VFS/Output/vfsroot-with-overlay.c.tmp/cache',
'type': 'directory', ... },
So it makes sense to me that we need to handle both kinds of absolute path.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1448-1449
+ // which style we have, and use it consistently.
+ if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
+ path_style = sys::path::Style::posix;
+ } else if (sys::path::is_absolute(Name, sys::path::Style::windows)) {
----------------
I wonder if there's a simpler fix here. If the working directory is an absolute
Windows-style path, we could prepend the drive letter of the working directory
onto any absolute Posix style paths read from YAML files. That's somewhat
consistent with what native Windows tools do. In cmd, you can run `cd
\Windows`, and that will mean `C:\Windows` if the active driver letter is C. I
think on Windows every drive has an active directory, but that's not part of
the file system model.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70701/new/
https://reviews.llvm.org/D70701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
