[libcxx-commits] [PATCH] D91171: [16/N] [libcxx] Implement the permissions function for windows

[Adrian McCarthy via Phabricator via libcxx-commits]

amccarth requested changes to this revision.
amccarth added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/src/filesystem/operations.cpp:1434
+    return err.report(detail::make_windows_error(GetLastError()));
+  if (attributes & FILE_ATTRIBUTE_REPARSE_POINT && resolve_symlinks) {
+    detail::WinHandle h(p.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES,
----------------
Can you add a comment explaining why reparse points need special handling?

Both branches appear to do the same thing using different APIs.  I assume this has to do with whether you're tweaking the reparse point itself or the file it points to, but that's not super obvious.

(And if that is the reason, then I'd expect these branches to be the other way around.)


================
Comment at: libcxx/src/filesystem/operations.cpp:1445
+      basic.FileAttributes |= FILE_ATTRIBUTE_READONLY;
+    if (!SetFileInformationByHandle(h, FileBasicInfo, &basic, sizeof(basic)))
+      return err.report(detail::make_windows_error(GetLastError()));
----------------
Is it worth skipping the Set operation if the file (or reparse point) has no effect?

I imagine a common thing to do is to set permissions on zillions of files at once, so avoiding an extra system call when its not necessary seems like it would be worth the cost of an extra conditional branch.


================
Comment at: libcxx/src/filesystem/operations.cpp:1451
+      attributes |= FILE_ATTRIBUTE_READONLY;
+    if (!SetFileAttributesW(p.c_str(), attributes))
+      return err.report(detail::make_windows_error(GetLastError()));
----------------
Same question about skipping the Sets when they're not necessary.


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

https://reviews.llvm.org/D91171