[PATCH] D70701: Fix more VFS tests on Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a subscriber: JDevlieghere.
rnk added a comment.
This revision is now accepted and ready to land.

+ at JDevlieghere, due to recent VFS test fixup.

I think this looks good, and in the absence of input from other VFS stakeholders, let's land this soon. Thanks!



================
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:
> amccarth wrote:
> > amccarth wrote:
> > > 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.
> > To make sure I wasn't misremembering or hallucinating, I double-checked the behavior here:  Posix-style paths like `\tests` are not considered absolute paths in Windows-style.
> I see, I agree, the platforms diverge here:
>   bool rootDir = has_root_directory(p, style);
>   bool rootName =
>       (real_style(style) != Style::windows) || has_root_name(p, style);
> 
>   return rootDir && rootName;
> 
> So, on Windows rootDir is true and rootName is false.
> 
> I still wonder if this behavior shouldn't be pushed down into the base class. If I pass the path `\test` to the real `FileSystem::makeAbsolute` on Windows, should that prepend the CWD, or should it leave the path alone?
I think we discussed this verbally, and decided we should prepend the CWD, as is done here.


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

https://reviews.llvm.org/D70701