[PATCH] D30668: Add llvm::sys::fs::real_path

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.





================
Comment at: llvm/include/llvm/Support/FileSystem.h:347
+/// @param output The location to store the resolved path.
+/// @param shell_expand If true, expands shell-specific constructs such as ~
+///                     (on posix platforms).
----------------
I think this description is very vague. After reading this, I would expect we will handle all kinds of fancy shell expansions (`${HOME//foo}` anyone ?). I suggest name the argument `tilde_expand`, document it as performing home-directory expansion using `~` and nothing more, until we have a very good idea of what other kinds of expansions we may want to perform.


================
Comment at: llvm/lib/Support/Unix/Path.inc:845
+  std::error_code EC;
+  if (S[0] == '~') {
+    // open() doesn't accept tilde expressions, they are expanded by the shell.
----------------
zturner wrote:
> rnk wrote:
> > labath wrote:
> > > I am wondering whether the tilde expansion should not be controlled by an argument. As the comment says, tilde expansion is done at a completely different level than path canonicalization. `~` is a perfectly valid path component and it's not clear to me that every user of this function would want the magic tilde expansion behavior as well. I think that can create some confusion, as this is not the behavior of any of the standard utilities and functions elsewhere (`realpath` unix utility, `realpath` POSIX function, llvm's `openFileForRead`, ...). For that reason it may be safer for the caller to explicitly opt-in into this behavior.
> > Does the tilde expansion work on Windows? If not, let's leave it out like Pavel suggests.
> No, but I thought he just suggested to make it opt-in rather than leave it out entirely.  The "equivalent" thing on Windows would be to expand environment variables.  That said, maybe there's a case to be made for allowing tilde expansion on Windows too.  I mean, Windows users do have a home directory, so it's not like it doesn't make sense at all.
Yes, my suggestion was to make this opt-in. I can see a case for making ~-expansion a separate function, but that may then be clunky to use for cases when you want both, and we will probably end up with a wrapper function that does both sooner or later.

PS: I like the idea of `~` working on windows as well (we just shouldn't be pretending to emulate any specific shell then).


https://reviews.llvm.org/D30668