[PATCH] D54448: [FileSystem] Add expand_tilde function

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/lib/Support/Unix/Path.inc:554
+
+std::error_code expand_tilde(const Twine &path, SmallVectorImpl<char> &dest) {
+  dest.clear();
----------------
sammccall wrote:
> 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.
I don't know the history/reason for the current implementation so I don't feel confident adding a comment. 


================
Comment at: llvm/unittests/Support/Path.cpp:536
+  bool Result = llvm::sys::path::home_directory(HomeDir);
+  if (Result) {
+    ASSERT_NO_ERROR(fs::expand_tilde(HomeDir, Expected));
----------------
sammccall wrote:
> is there any reason for this to fail? Maybe assert it instead so the test doesn't silently become a no-op?
Added a comment.


https://reviews.llvm.org/D54448