[PATCH] D54435: [VFS] Add "expand tilde" argument to getRealPath.

[Sam McCall via llvm-commits]

Thanks for sending this!
I think fs::real_path isn't a great precedent to be following here, but
that we can put the same functionality behind a slightly different API.

I may well be missing requirements, details, or historical context here.
Hopefully you or someone can correct me if so!

On Mon, Nov 12, 2018, 20:06 Jonas Devlieghere via Phabricator <
reviews at reviews.llvm.org wrote:

> JDevlieghere added a comment.
>
> In https://reviews.llvm.org/rL346453#296733, @sammccall wrote:
>
> > I think this change was landed without review, and it's a complicated
> interface change, so I'm going to revert it for now.
> >
> > I also have some specific concerns about the design:
> >
> > - this doesn't seem to be a per-filesystem concern. e.g. in an overlaid
> filesystem, whose responsibility is it to resolve ~? It's not clear this
> belongs in the virtualization layer at all.
>
>
> I'm not sure this is true. It the overlay specifies a different home
> directory (this functionality doesn't exist yet)

>From what I can tell, expanding ~ as the home directory path doesn't really
have anything to do with the filesystem per session, it's a shell feature
that interacts with the environment (just like expanding $PWD). So overlays
don't get to specify a home directory, it's always $HOME.

And only tools that act like shells are really expected to do this
expansion: this includes LLDB but not e.g. tools that take filenames on the
CLI. (Otherwise we get double-expansion).

As such it seems like this should be two orthogonal function calls:
 - one to expand ~ (and possibly ~user, $FOO) etc, this is OS-specific but
not FS-specific and so not part of VFS (FS and VFS users can share this)
 - realPath as it is today (which matches the real_path posix call it
models)

This is both more orthogonal (particularly important for VFS, with multiple
implementations) and more accurately places this as a somewhat-niche
path-transformation feature, rather than a core FS thing.

> - implementing this is a substantial burden on VFSes, many of which (by
> their nature) live out-of-tree. It's not clear what the consequences of
> ignoring the parameter are, and what callers can expect.
> > - use of optional-args in virtual methods would be nice to avoid
> > - the use of boolean params like this in general doesn't scale well
>
> Would a separate method address those 3 concerns?

Yes! Though I don't the addition should be a VFS method, rather probably a
function in llvm::sys::path:: (the implementation actually exists in
Unix/Path.inc already).

My

goal was to keep things simple and mirror llvm's API, which helps with
> migrating between the two.

For the same reasons I think the llvm API is a mistake (though it's less
harmful, as it's not an interface with many implementations).

If consistency is important, maybe we can split this apart too, but I think
real_path(p, true) <=> VFS->realPath(shellExpand(p)) is a simple enough
transformation if we document it.

I don't have a strong opinion on this though, as long as the functionality
> is available.
>
> > Happy to help with review if you do want to move forward with this!
>
> Thanks, I look forward to your input!
>
Let me know what you think!
Cheers, Sam

>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181112/2344029c/attachment.html>