Page MenuHomePhabricator

[FileSystem] Add expand_tilde function
ClosedPublic

Authored by JDevlieghere on Nov 12 2018, 2:07 PM.

Details

Summary

In D54435 there was some discussion about the expand_tilde flag for real_path that I wanted to expose through the VFS. The consensus is that these two things should be separate functions. Since we already have the code for this I went ahead and added a function expand_tilde that does just that.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Nov 12 2018, 2:07 PM

Update unit test.

zturner added inline comments.Nov 12 2018, 2:41 PM
llvm/unittests/Support/Path.cpp
532–534 ↗(On Diff #173762)

I don't think we need these lines?

548 ↗(On Diff #173762)

Or this line?

Remove useless copy-pasted lines from unit test.

JDevlieghere marked 2 inline comments as done.Nov 12 2018, 2:50 PM

Thanks! Only real substantial suggestion is not to require handling an error where none is possible.

llvm/include/llvm/Support/FileSystem.h
352 ↗(On Diff #173772)

it'd be worth explicitly stating whether ~user is handled, whether $ENVVAR is handled, and what this does on windows.

355 ↗(On Diff #173772)

this can't fail, remove the error_code return?

then it can return its result, which makes it a bit easier to use.

llvm/lib/Support/Unix/Path.inc
554 ↗(On Diff #173772)

the much richer alternative on unix is to wrap wordexp(), which more precisely emulates a shell.
I can imagine there are reasons not to do that (e.g. quoting surprises, performance), but it may be worth a comment.

llvm/unittests/Support/Path.cpp
536 ↗(On Diff #173772)

is there any reason for this to fail? Maybe assert it instead so the test doesn't silently become a no-op?

538 ↗(On Diff #173772)

also test ~/foo?

(would be nice to have a test for ~username on unix, but not sure if it's easy to make that work)

JDevlieghere marked 4 inline comments as done.

Thanks for the review Sam!

llvm/lib/Support/Unix/Path.inc
554 ↗(On Diff #173772)

I don't know the history/reason for the current implementation so I don't feel confident adding a comment.

llvm/unittests/Support/Path.cpp
536 ↗(On Diff #173772)

Added a comment.

sammccall accepted this revision.Nov 13 2018, 9:34 AM
This revision is now accepted and ready to land.Nov 13 2018, 9:34 AM
This revision was automatically updated to reflect the committed changes.