rnk added inline comments.
================
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:
> 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.
I see, I agree, the platforms diverge here:
bool rootDir = has_root_directory(p, style);
bool rootName =
(real_style(style) != Style::windows) || has_root_name(p, style);
return rootDir && rootName;
So, on Windows rootDir is true and rootName is false.
I still wonder if this behavior shouldn't be pushed down into the base class.
If I pass the path `\test` to the real `FileSystem::makeAbsolute` on Windows,
should that prepend the CWD, or should it leave the path alone?
================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
- if (IsRootEntry && !sys::path::is_absolute(Name)) {
- assert(NameValueNode && "Name presence should be checked earlier");
----------------
amccarth wrote:
> rnk wrote:
> > Is there a way to unit test this? I see some existing tests in
> > llvm/unittests/Support/VirtualFileSystemTest.cpp.
> >
> > I looked at the yaml files in the VFS tests this fixes, and I see entries
> > like this:
> > { 'name': '/tests', 'type': 'directory', ... },
> > { 'name': '/indirect-vfs-root-files', 'type': 'directory', ... },
> > { 'name': 'C:/src/llvm-project/clang/test/VFS/Inputs/Broken.framework',
> > 'type': 'directory', ... },
> > { 'name':
> > 'C:/src/llvm-project/build/tools/clang/test/VFS/Output/vfsroot-with-overlay.c.tmp/cache',
> > 'type': 'directory', ... },
> >
> > So it makes sense to me that we need to handle both kinds of absolute path.
> > Is there a way to unit test this?
>
> What do you mean by "this"? The `/tests` and `/indirect-vfs-root-files`
> entries in that yaml are the ones causing several tests to fail without this
> fix, so I take it that this is already being tested. But perhaps you meant
> testing something more specific?
> What do you mean by "this"?
I guess what I meant was, can you unit test the whole change in case there are
behavior differences here not covered by the clang tests?
================
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:
> 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.
The way in which I see it as being "simpler" is that all absolute paths would
end up having drive letters on Windows. I was hoping that would avoid some
corner cases that arise from having a virtual file system tree in memory where
some files are rooted in a drive letter and some appear in non-drive letter top
level directories from Posix paths.
In any case, I think the change you have here is definitely correct, and is a
smaller change in behavior. The path component iterators below need to use the
correct style.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70701/new/
https://reviews.llvm.org/D70701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
