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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
