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

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

Quuxplusone added inline comments.


================
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:
> Quuxplusone wrote:
> > 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;
> > }
> > ```
> Sure, I can inline it.
> 
> > If you're only normalizing one side, I'd name the function PermsNormalizeExpected (since you could perhaps have chosen to implement a PermsNormalizeActual instead).
> 
> Hmm, so no change wrt that as we'd inline it? Should we name the comparison function in a way to highlight this too, e.g. `PermsEqNormalizeExpected` (which is a mouthful...)? The tricky thing is that it's not clear to the callers which argument is which.
Meh. It has only a few callers, so IMHO the approach I presented for `PermsEq` is fine.
If you were going to rewrite it, though, the next thing I'd suggest would be
```
    TEST_CHECK(pp == NormalizeExpectedPerms(TC.expected));
```
at the call-sites.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101728