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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

I can see why it's probably not feasible to test `realPathFromHandle` and `directoryRealPath` directly, but is there a way to test `real_path` to ensure that it covers a lot of the important cases (like MAX_PATH on Windows)?



================
Comment at: llvm/lib/Support/Unix/Path.inc:480
 
+static void expand_tilde_expr(const Twine &Path, SmallVectorImpl<char> &Dest) {
+  SmallString<64> ResultStorage;
----------------
Isn't the style `expandTildeExpr`?


================
Comment at: llvm/lib/Support/Unix/Path.inc:837
 
+std::error_code real_path(const Twine &path, SmallVectorImpl<char> &dest) {
+  int fd;
----------------
`realPath`?


================
Comment at: llvm/lib/Support/Unix/Path.inc:838
+std::error_code real_path(const Twine &path, SmallVectorImpl<char> &dest) {
+  int fd;
+  dest.clear();
----------------
I would move the definition of `fd` down near the definition of `EC` so that it's not live before it needs to be.


================
Comment at: llvm/lib/Support/Unix/Path.inc:847
+    // open() doesn't accept tilde expressions, they are expanded by the shell.
+    // So expand then first before trying to resolve the file.
+    expand_tilde_expr(path, dest);
----------------
s/then/them/


================
Comment at: llvm/lib/Support/Windows/Path.inc:792
+  RealPath.clear();
+  wchar_t RealPathUTF16[MAX_PATH];
+  DWORD CountChars = ::GetFinalPathNameByHandleW(H, RealPathUTF16, MAX_PATH,
----------------
Maybe this should be larger than MAX_PATH because it will have the `\\?\` prefix.


================
Comment at: llvm/lib/Support/Windows/Path.inc:793
+  wchar_t RealPathUTF16[MAX_PATH];
+  DWORD CountChars = ::GetFinalPathNameByHandleW(H, RealPathUTF16, MAX_PATH,
+                                                 FILE_NAME_NORMALIZED);
----------------
Reading MSDN, it looks like the length of the buffer should be reported as `MAX_PATH - 1`, otherwise a path that's exactly the maximum length might result in a one character overrun.


================
Comment at: llvm/lib/Support/Windows/Path.inc:794
+  DWORD CountChars = ::GetFinalPathNameByHandleW(H, RealPathUTF16, MAX_PATH,
+                                                 FILE_NAME_NORMALIZED);
+  if (CountChars > 0 && CountChars < MAX_PATH) {
----------------
GetFinalPathNameByHandle is Vista+, no XP.  I assume that's OK.


================
Comment at: llvm/lib/Support/Windows/Path.inc:804
+  }
+}
+
----------------
Looks like this silently fails for paths longer than MAX_PATH.  Is that a limitation we're already facing in the rest of path support in LLVM?  Should this return an error in that case?


================
Comment at: llvm/lib/Support/Windows/Path.inc:814
+  HANDLE H =
+      ::CreateFileW(PathUTF16.begin(), GENERIC_READ,
+                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
----------------
Would PathUTF16.data() be more appropriate than .begin()?


================
Comment at: llvm/lib/Support/Windows/Path.inc:819
+    return mapWindowsError(GetLastError());
+  realPathFromHandle(H, RealPath);
+  ::CloseHandle(H);
----------------
If realPathFromHandle could return an error in the long name case, then you could propagate the error out here.


https://reviews.llvm.org/D30668