[libcxx-commits] [PATCH] D101728: [libcxx] [test] Fix filesystem permission tests for windows

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % comment.



================
Comment at: libcxx/test/support/filesystem_test_helper.h:621
+inline bool PermsEq(fs::perms Actual, fs::perms Expected) {
+  return Actual == PermsNormalize(Expected);
+}
----------------
mstorsjo wrote:
> I guess it can be argued that we should normalize both operands, as it's not evident to the callers about which parameter is which. But by only normalizing the expectation, we keep the test a bit stronger. But I can be convinced otherwise too.
If you're only normalizing one side, I'd name the function `PermsNormalizeExpected` (since you could perhaps have chosen to implement a `PermsNormalizeActual` instead). However, it's also a function with only one caller, so I'd actually inline it. I'd say
```
inline bool PermsEq(fs::perms Actual, fs::perms Expected) {
#ifdef _WIN32
  // On Windows, fs::perms maps down to one bit stored in the filesystem,
  // a boolean readonly flag.
  // Normalize the expected permissions to the format that is returned:
  // all files are read+exec for all users; writable files are writable for all users.
  Expected |= fs::perms::owner_read | fs::perms::group_read | fs::perms::others_read;
  Expected |= fs::perms::owner_exec | fs::perms::group_exec | fs::perms::others_exec;
  if (Expected & (fs::perms::owner_write | fs::perms::group_write | fs::perms::others_write))
    Expected |= fs::perms::owner_write | fs::perms::group_write | fs::perms::others_write;
#endif
  return Actual == Expected;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101728