[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth marked 2 inline comments as done.
amccarth added a comment.

A (hopefully) more cogent response than my last round.  I'm still hoping to hear from VFS owners.



================
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 {};
----------------
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.


================
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)) {
----------------
amccarth wrote:
> 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.
> ... I don't think prepending the drive onto absolution Posix style paths read from YAML would work....

I misspoke.  The idea of prepending the drive onto absolute Posix-style paths read from the YAML probably could be made to work for the common cases like the ones in these tests.

But I still don't think that approach would simplify anything.

Furthermore, I think it could open a potential Pandora's box of weird corner cases.  For example, in a system with multiple drives, the current drive may not always be the "correct" one to use.  Slapping a drive letter onto a Posix-style path creates a Frankenstein hybrid that's neither Windows-style nor Posix-style.  Doing so because we know the subsequent code would then recognize it as an absolute path seems like a way to create an unnecessary coupling between the VFS YAML parser and the LLVM path support.

In my mind, the model here is that these roots can be in either style.  I prefer to let the code explicitly reflect that fact rather than trying to massage some of the paths into a form that happens to cause the right outcome.


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

https://reviews.llvm.org/D70701