BTW, I hope I didn't come across as overly negative in my previous
response. I'd love to see the situation improved. I just don't know what
that would look like.
> One thing I see, but wasn't obvious from your description, is that the
mixed/hybrid separator behaviour only happens for `defined(_WIN_32)`.
> [copy of `is_separator` elided]
It's a runtime decision based on the specified style, not a compile-time
one based on _WIN_32. If the caller of `is_separator` passes
`Style::windows` then it'll accept either `/` or `\\`, even if LLVM was
compiled and run on Linux or Mac. I believe there's a VFS test that
redirects a Windows-style path to a Posix-style one (an in-memory file
system), and that test passes on both kinds of hosts.
But I get the gist of the point. My feeling is that, unless we can
eliminate hybrid styles, Paths should support a Style::hybrid. It would be
messy because more ambiguities about how to map things crop up.
> Honestly, I'm not sure we have a good definition of what makeAbsolute
> should do.
I should have said: **I** don't have a good understanding of what
makeAbsolute should do. Even saying it should prepend the current working
directory is an incomplete answer. On Windows, a process can have multiple
current directories: one for each drive. And a process has one current
drive. So the current working directory is the current directory for the
current drive. A Windows path like "D:foo.txt" is a relative path.
Literally prepending the current working directory gives us
`C:\users\me\D:foo.txt`, which is syntactically wrong. But even if we're
smart enough to fix up the syntax, we'd get `C:\users\me\foo.txt` or
`D:\users\me\foo.txt`, both of which would (likely) be wrong. The way the
OS would resolve it is to look up the process's current directory for the
D: drive, and insert that into the missing bit.
Anyway, I look forward to any and all proposals to improve this situation.
On Thu, Jan 21, 2021 at 6:47 PM Duncan P. N. Exon Smith via Phabricator <
revi...@reviews.llvm.org> wrote:
> dexonsmith added a comment.
>
> Thanks for the quick and detailed response!
>
> Your explanation of hybrid behaviour on Windows was especially helpful, as
> I didn't fully understand how that worked before.
>
> One thing I see, but wasn't obvious from your description, is that the
> mixed/hybrid separator behaviour only happens for `defined(_WIN_32)`. E.g.,
> from Path.cpp:
>
> bool is_separator(char value, Style style) {
> if (value == '/')
> return true;
> if (real_style(style) == Style::windows)
> return value == '\\';
> return false;
> }
>
>
>
> In D70701#2514143 <https://reviews.llvm.org/D70701#2514143>, @amccarth
> wrote:
>
> >> - The path style is detected when reading the YAML file (as now).
> >
> > Which path's style? The virtual one that's going to be redirected or the
> > actual one it's redirected at?
>
> Both, but you've mostly convinced me not to go down this route.
>
> > - Paths in the YAML file are canonicalized to native at parse time.
> >
> > If we canonicalize the virtual path, VFS would no longer be valuable for
> > creating platform-agnostic tests.
>
> This is a good point I hadn't considered.
>
> > In LLDB, we have the additional wrinkle of remote debugging, where the
> > debugger may be running on a Windows machine while the program being
> > debugged is running on a Linux box. You always have to know whether a
> path
> > will be used on the debugger host or the debuggee host. And there are
> > similar concerns for post-mortem debugging from a crash collected on a
> > different type of host.
>
> Ah, interesting.
>
> > Honestly, I'm not sure we have a good definition of what makeAbsolute
> > should do.
>
> Perhaps the name isn't ideal --
> `prependRelativePathsWithCurrentWorkingDirectory()` would be more precise
> -- but otherwise I'm not sure I fully agree. Regardless, I acknowledge your
> point that the two worlds are awkwardly different.
>
> I'm going to think about other options; thanks again for your feedback. I
> am still leaning toward `FileSystem::makeAbsolute()` not being virtual /
> overridden, but I have a better idea of how to approach that. One idea is
> for the `RedirectingFileSystem` to keep track of where different styles
> were used when parsing.... I'm not sure if that'll pan out though.
>
>
> Repository:
> rG LLVM Github Monorepo
>
> 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
