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