[libcxx-commits] [PATCH] D91143: [11/N] [libcxx] Hook up a number of operation functions to their windows counterparts

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:446
+  if (GetLastError() != ERROR_INVALID_PARAMETER)
+    return set_errno();
+  if (CreateSymbolicLinkW(newname, oldname, flags))
----------------
amccarth wrote:
> Technically, there's nothing wrong here.  It just _feels_ a little sketchy to call `GetLastError()` twice in a row.
Yeah, it's not ideal, but then again, making a separate version of the `set_errno()` function that takes an error code as input, just for this case, also feels a bit superfluous (as the first call to GetLastError() shouldn't clobber the first one). Or would a default parameter, like `set_errno(int err = GetLastError())` work?


================
Comment at: libcxx/src/filesystem/operations.cpp:467
+int remove(const wchar_t *path) {
+  detail::WinHandle h(path, DELETE, FILE_FLAG_OPEN_REPARSE_POINT);
+  if (!h)
----------------
amccarth wrote:
> Why not implement this with `::DeleteFile()`?  Does it have to do with waiting until the last handle is closed?
Haven't considered/tested that, I can try that and run it over the testcases to see if it works.

The reason I ended up with this, was that I first just called `_wremove()`, followed by a `_wrmdir()` if the first failed and the path is a directory - as that was the most obvious path forward when porting over from the posix remove() function. That worked mostly, but failed in some symlink cases, and then I peeked at how the MS STL (which has the same license as libcxx, fwiw) implemented it, and I did the same.


================
Comment at: libcxx/src/filesystem/operations.cpp:489
+  HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  return truncate_handle(h, length);
+}
----------------
amccarth wrote:
> I'm mildly concerned about the way files are sometimes manipulated by HANDLE and sometimes by file descriptor.
> 
> If `fd` represents a file in text mode, then the caller needs to be very careful to understand that `length` must be the _byte_ offset, not the _char_ offset into the file.
File descriptors here are only an internal detail, fwiw, they're only used within the `__copy_file` function. But good catch about text vs binary mode; I guess the calls to `FileDescriptor::create_with_status` need to be amended with `O_BINARY`.


================
Comment at: libcxx/src/filesystem/operations.cpp:501
+  if (!(MoveFileExW(from, to,
+                    MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING)))
+    return set_errno();
----------------
amccarth wrote:
> Consider also `MOVEFILE_WRITE_THROUGH` for the case that it is indeed copied and then (probably) deleted.
Sounds like a good thing to have, I can add that. These flags were based on what the MS STL use, and I didn't see that flag used there though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91143