[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 marked 3 inline comments as done.
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))
----------------
mstorsjo wrote:
> 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?
Changed `set_errno()` to take the error code as a parameter, with the parameter set by default to `GetLastError()`.


================
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)
----------------
mstorsjo wrote:
> 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.
I tried with this implementation, but this still fails on removing symlinks to directories:
```
int remove(const wchar_t *path) {
  if (DeleteFileW(path))
    return 0;
  int e = GetLastError();
  if (e == ERROR_ACCESS_DENIED) {
    if (is_directory(path)) {
      if (!RemoveDirectoryW(path)) 
        return set_errno();
      return 0;
    }
  }
  return set_errno(e);
}
```



================
Comment at: libcxx/src/filesystem/operations.cpp:489
+  HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  return truncate_handle(h, length);
+}
----------------
mstorsjo wrote:
> 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`.
Adding `O_BINARY` at the callsites that open file descriptors.


================
Comment at: libcxx/src/filesystem/operations.cpp:501
+  if (!(MoveFileExW(from, to,
+                    MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING)))
+    return set_errno();
----------------
mstorsjo wrote:
> 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.
Adding that flag as it sounds beneficial, despite MS STL not using it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91143