[PATCH] D68953: Enable most VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

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

Somehow Phabricator failed to notify me that you'd already left comments.  I even searched my inbox to see if I'd just missed the notification.  Nope.  Nothing.  I came here today to ping you for the review and discovered you'd already done your part.

Anyway, I'll update the patch and ping again when that's done.

Thanks.



================
Comment at: clang/lib/Lex/HeaderSearch.cpp:782-783
       // Concatenate the requested file onto the directory.
-      // FIXME: Portability.  Filename concatenation should be in sys::Path.
-      TmpDir = IncluderAndDir.second->getName();
-      TmpDir.push_back('/');
-      TmpDir.append(Filename.begin(), Filename.end());
+      TmpPath = IncluderAndDir.second->getName();
+      llvm::sys::path::append(TmpPath, Filename);
 
----------------
rnk wrote:
> I'm surprised this just works. :) You ran the whole clang test suite, and nothing broke, right? I would've expected something to accidentally depend on this behavior.
Yeah, I recall it all working before, but, double-checking today, I'm seeing some collateral damage.  Investigating.


================
Comment at: clang/test/VFS/external-names.c:4-5
 
 // FIXME: PR43272
 // XFAIL: system-windows
 
----------------
rnk wrote:
> You made changes to this file, but it's still XFAILed. Is that intentional?
The change removes one obstacle, but the test still fails on Windows because there's still an outstanding bug.  If you like, I can hoist this change out until I have a patch to fix the outstanding issue.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1839
   SmallVector<StringRef, 8> Components;
-  Components.push_back("/");
+  Components.push_back(sys::path::get_separator());
   getVFSEntries(*RootE, Components, CollectedEntries);
----------------
rnk wrote:
> I think this will ultimately produce paths that look like: `\C:\foo\bar\baz`. Ultimately, we loop over the components above and repeatedly call sys::path::append on them.
> 
> Clang only uses this function for collecting module dependency info, it seems. Maybe that's what the remaining failures are about?
I didn't encounter anything like `\C:\foo`.  Perhaps the (enabled) VFS tests all prime the roots so that never happens here.  I'll see if I can add a test to uncover that problem.

And, yes, use of this function seems limited to reading VFS paths from the YAML, so its scope is pretty limited.  At least some of the remaining problems have to do with the fact that, while the target paths can always be expressed in the host style, the key paths can be in either Posix or Windows format, which leads to confusion when you try to get answers from functions like `sys::path::is_absolute` where you have to pick the right style to get the right answer.


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

https://reviews.llvm.org/D68953