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

[Adrian McCarthy via Phabricator via libcxx-commits]

amccarth added a comment.

I have a couple questions to consider inline.  If those have already been thought through, then LTGM.



================
Comment at: libcxx/src/filesystem/operations.cpp:446
+  if (GetLastError() != ERROR_INVALID_PARAMETER)
+    return set_errno();
+  if (CreateSymbolicLinkW(newname, oldname, flags))
----------------
Technically, there's nothing wrong here.  It just _feels_ a little sketchy to call `GetLastError()` twice in a row.


================
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)
----------------
Why not implement this with `::DeleteFile()`?  Does it have to do with waiting until the last handle is closed?


================
Comment at: libcxx/src/filesystem/operations.cpp:489
+  HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  return truncate_handle(h, length);
+}
----------------
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.


================
Comment at: libcxx/src/filesystem/operations.cpp:501
+  if (!(MoveFileExW(from, to,
+                    MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING)))
+    return set_errno();
----------------
Consider also `MOVEFILE_WRITE_THROUGH` for the case that it is indeed copied and then (probably) deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91143