[PATCH] D75372: [Support] Improve Windows widenPath and add support for long UNC paths

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

I'm not sure I agree with moving widen from `sys::path` to `sys::fs`.  Is there consensus on the layering here?  This adds dependencies for `sys::fs` on `sys::path`, but I would expect the dependencies to be in the other direction--the path style you use would depend on the underlying file system, not the other way around, right?  The long path prefix on an absolute local path depends on the underlying file system being NTFS.

And, in my recent experience `sys::path::is_absolute(..., sys::path::style::windows)` seems buggy with UNC paths, so I'm skeptical if it works right with the long path prefix.



================
Comment at: llvm/lib/Support/Windows/Path.inc:82
+  size_t MaxDirLen =
+      ForCreateDir        // Win32 CreateDirectory API has a lower limit.
+          ? MAX_PATH - 12 // Must leave room for 8.3 filename.
----------------
rnk wrote:
> Why not always use the lower limit? What is the harm in using the UNC prefixes slightly more often?
The long path prefix isn't a UNC prefix.  The long path prefix is an escape that tells many system APIs not to try to interpret the string as a file system path, but simply to pass it on to the underlying file system as an opaque string.  That's great if you underlying file system is NTFS, that's great.  I don't know whether something like a FAT32 flash drive handles that.  If not, then there could be some harm to prematurely appending the long path prefix.  That's probably pretty low risk.

I think the `ForCreateDir` parameter makes this API a leaky abstraction and the risk is low.  If you insist on having a way to enforce different limits, then I'd make the parameter be the path limit you want, default it to `MAX_PATH`, and let the special cases (like code that calls CreateDirectoryW) explain why they're setting it lower.


================
Comment at: llvm/lib/Support/Windows/Path.inc:117
+  // name and long paths must use '\' as the separator.
+  if (Path8Str[RootName.size()] == '/')
+    Path8Str[RootName.size()] = '\\';
----------------
Would it be appropriate to check (or at least assert) that `RootName.size() <= Path8Str.size()`?  If somehow the entire input string could be considered the root name, then this reference would be out of bounds.


================
Comment at: llvm/unittests/Support/Path.cpp:674
 #ifdef _WIN32
   // Path name > 260 chars should get an error.
   const char *Path270 =
----------------
Does this patch, which allows longer paths on Windows, invalidate this test which checks that `PATH_MAX` is enforced?  Presumably, `createUniqueFile` could use the new `widen`.  Or is the issue here that `PATH_MAX` also applied to each component of a path?


================
Comment at: llvm/unittests/Support/Path.cpp:1880
+TEST_F(FileSystemTest, widenPath) {
+  std::wstring LongPathPrefix(L"\\\\?\\");
+  SmallVector<wchar_t, MAX_PATH + 16> Result;
----------------
`const`?


================
Comment at: llvm/unittests/Support/Path.cpp:1903
+  // Construct the expected result.
+  ASSERT_NO_ERROR(windows::UTF8ToUTF16(Input, Expected));
+  Expected.insert(Expected.begin(), LongPathPrefix.begin(),
----------------
This seems like a better place to define `Expected`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75372/new/

https://reviews.llvm.org/D75372