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

[Andrew Ng via Phabricator via llvm-commits]

andrewng marked 11 inline comments as done.
andrewng added inline comments.


================
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.
----------------
amccarth wrote:
> 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.
I think anything to help avoid having to mangle the input is beneficial. The need for this type of code is already bad enough.

The real purpose of this function is to best prepare the path for the Win32 file APIs. It's not meant to be any kind of validation. So the Win32 file APIs could still fail for various reasons, such as other file system constraints.

I like the idea to change `ForCreateDir` to `MaxPathLen`, thanks.


================
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()] = '\\';
----------------
amccarth wrote:
> 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.
Thanks for spotting this one!


================
Comment at: llvm/unittests/Support/Path.cpp:674
 #ifdef _WIN32
   // Path name > 260 chars should get an error.
   const char *Path270 =
----------------
amccarth wrote:
> 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?
As you've said, I believe that there is also a length limit to each component of a path, although I'm not sure what that exact limit is.


================
Comment at: llvm/unittests/Support/Path.cpp:1880
+TEST_F(FileSystemTest, widenPath) {
+  std::wstring LongPathPrefix(L"\\\\?\\");
+  SmallVector<wchar_t, MAX_PATH + 16> Result;
----------------
amccarth wrote:
> `const`?
My usual style is to liberally use `const` where appropriate. But I've had objections to that in the past. As you've mentioned it here, I'll go back to my usual style and see if anyone objects.


================
Comment at: llvm/unittests/Support/Path.cpp:1887
+  std::string Input("C:\\foldername\\");
+  std::string Pi("π"); // UTF-8: 0xCF 0x80
+  // Add Pi up to the MAX_PATH limit.
----------------
rnk wrote:
> Is this the first introduction of a real UTF-8 source character? Are you confident that this will work portably? Do you need to write `u8"pi"` to make this work where utf-8 is not the local codepage?
Good point. It should be OK for all supported Win32 compilers, but to be on the safe side, I'll change it to hex representation to be sure.


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

https://reviews.llvm.org/D75372