This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix filesystem permission tests for windows
ClosedPublic

Authored by mstorsjo on May 2 2021, 1:29 PM.

Details

Summary

On Windows, the permission bits are mapped down to essentially only
two possible states; readonly or readwrite. Normalize the checked
permission bitmask to match what the implementation will return.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.May 2 2021, 1:29 PM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2021, 1:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo added inline comments.May 2 2021, 2:33 PM
libcxx/test/support/filesystem_test_helper.h
621

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.

Quuxplusone added a subscriber: Quuxplusone.

LGTM % comment.

libcxx/test/support/filesystem_test_helper.h
621

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;
}
mstorsjo added inline comments.May 5 2021, 2:36 PM
libcxx/test/support/filesystem_test_helper.h
621

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.

Quuxplusone added inline comments.May 5 2021, 2:56 PM
libcxx/test/support/filesystem_test_helper.h
621

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.

mstorsjo added inline comments.May 5 2021, 9:10 PM
libcxx/test/support/filesystem_test_helper.h
621

Ah, that's probably even clearer, I'll update to that form in a moment.

mstorsjo updated this revision to Diff 343307.May 6 2021, 12:18 AM

Removed the unnecessary PermsEq, renamed the normalize function to NormalizeExpectedPerms.

Ping for second approval

@Mordante - does this look acceptable to you?

Mordante accepted this revision.May 11 2021, 10:15 AM

LGTM, one small question.

libcxx/test/support/filesystem_test_helper.h
614

Is it specified which of the 3 write flags Windows sets when a file isn't read-only? Or should it set all 3 of them?

This revision is now accepted and ready to land.May 11 2021, 10:15 AM
mstorsjo added inline comments.May 11 2021, 10:19 AM
libcxx/test/support/filesystem_test_helper.h
614

I'm not sure if there's a spec somewhere; MSVC's STL sets all three in sync, and libc++ does the same. As we check whether our normalized expected flags are equal to what the implementation sets, this test will show if an implementation returns something else.