[PATCH] D68953: Enable most VFS tests on Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk added subscribers: arphaman, vsapsai, Bigcheese.
rnk added a comment.

+VFS people, mostly just to let them know
@arphaman @Bigcheese @vsapsai

Code changes all look good to me, but I had a bunch of questions.



================
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);
 
----------------
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.


================
Comment at: clang/test/VFS/external-names.c:4-5
 
 // FIXME: PR43272
 // XFAIL: system-windows
 
----------------
You made changes to this file, but it's still XFAILed. Is that intentional?


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1619-1620
 
   sys::path::const_iterator Start = sys::path::begin(Path);
   sys::path::const_iterator End = sys::path::end(Path);
   for (const auto &Root : Roots) {
----------------
After reading the code for a while, I see that drive letters are handled as just another component in the path, so I guess this code all works without explicitly considering drive letters.


================
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);
----------------
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?


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

https://reviews.llvm.org/D68953