<div dir="auto"><div>Thanks for sending this!</div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">I may well be missing requirements, details, or historical context here. Hopefully you or someone can correct me if so!</div><div dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr">On Mon, Nov 12, 2018, 20:06 Jonas Devlieghere via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">JDevlieghere added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/rL346453#296733" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/rL346453#296733</a>, @sammccall wrote:<br>
<br>
> I think this change was landed without review, and it's a complicated interface change, so I'm going to revert it for now.<br>
><br>
> I also have some specific concerns about the design:<br>
><br>
> - 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.<br>
<br>
<br>
I'm not sure this is true. It the overlay specifies a different home directory (this functionality doesn't exist yet)</blockquote></div></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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).</div><div dir="auto"><br></div><div dir="auto">As such it seems like this should be two orthogonal function calls:</div><div dir="auto"> - 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)</div><div dir="auto"> - realPath as it is today (which matches the real_path posix call it models)</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> - 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.<br>
> - use of optional-args in virtual methods would be nice to avoid<br>
> - the use of boolean params like this in general doesn't scale well<br>
<br>
Would a separate method address those 3 concerns?</blockquote></div></div><div dir="auto">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).</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> My</blockquote></div></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> goal was to keep things simple and mirror llvm's API, which helps with migrating between the two.</blockquote></div></div><div dir="auto">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).</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I don't have a strong opinion on this though, as long as the functionality is available.<br>
<br>
> Happy to help with review if you do want to move forward with this!<br>
<br>
Thanks, I look forward to your input!<br></blockquote></div></div><div dir="auto">Let me know what you think!</div><div dir="auto">Cheers, Sam</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
<br>
<br>
</blockquote></div></div></div>