[PATCH] D54448: [FileSystem] Add expand_tilde function

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

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



================
Comment at: llvm/include/llvm/Support/FileSystem.h:352
 
+/// Expands ~ expressions to the user's home directory.
+///
----------------
it'd be worth explicitly stating whether ~user is handled, whether $ENVVAR is handled, and what this does on windows.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:355
+/// @param path The path to resolve.
+/// @returns errc::success if the current path has been resolved in output,
+///          otherwise a platform-specific error_code.
----------------
this can't fail, remove the error_code return?

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


================
Comment at: llvm/lib/Support/Unix/Path.inc:554
+
+std::error_code expand_tilde(const Twine &path, SmallVectorImpl<char> &dest) {
+  dest.clear();
----------------
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.


================
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));
----------------
is there any reason for this to fail? Maybe assert it instead so the test doesn't silently become a no-op?


================
Comment at: llvm/unittests/Support/Path.cpp:538
+    ASSERT_NO_ERROR(fs::expand_tilde(HomeDir, Expected));
+    ASSERT_NO_ERROR(fs::expand_tilde("~", Actual));
+    EXPECT_EQ(Expected, Actual);
----------------
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)


https://reviews.llvm.org/D54448