amccarth marked 3 inline comments as done.
amccarth 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 {};
----------------
rnk wrote:
> 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.
Yes, it's necessary. The Posix-style path `\tests` is not considered absolute
on Windows. Thus the original makeAboslute would merge it with the current
working directory, which gives it a drive letter, like `D:\tests\`. The drive
letter component then causes comparisons to fail.
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
- if (IsRootEntry && !sys::path::is_absolute(Name)) {
- assert(NameValueNode && "Name presence should be checked earlier");
----------------
rnk wrote:
> 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.
> Is there a way to unit test this?
What do you mean by "this"? The `/tests` and `/indirect-vfs-root-files`
entries in that yaml are the ones causing several tests to fail without this
fix, so I take it that this is already being tested. But perhaps you meant
testing something more specific?
================
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)) {
----------------
rnk wrote:
> 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.
I'm not seeing how this would be simpler.
I don't understand the analogy of your proposal to what the native Windows
tools do. When you say, `cd \Windows`, the `\Windows` is _not_ an absolute
path. It's relative to the current drive.
I could be wrong, but I don't think prepending the drive onto absolution Posix
style paths read from YAML would work. That would give us something like
`D:/tests` (which is what was happening in makeAbsolute) and that won't match
paths generated from non-YAML sources, which will still come out as
`/tests/foo.h`.
> I think on Windows every drive has an active directory ....
Yep. I think of it as "every drive has a _current_ directory" and each process
has a current drive. When you want the current working directory, you get the
current directory of the current drive.
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
