Page MenuHomePhabricator

[libcxx] [test] Use GetWindowsInaccessibleDir() instead of dirs with perms::none in fs.op.is_*
ClosedPublic

Authored by mstorsjo on Mar 11 2021, 11:43 AM.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Mar 11 2021, 11:43 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 11:43 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius accepted this revision as: curdeius.Mar 11 2021, 11:42 PM
curdeius added a subscriber: curdeius.

LGTM.

Ping @Mordante (who reviewed a similar patch earlier) for second approval

mstorsjo updated this revision to Diff 335605.Tue, Apr 6, 11:18 AM

Amended to remove xfails and to allow the dir to be missing. Has 1 approval from earlier.

Quuxplusone added inline comments.
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_block_file/is_block_file.pass.cpp
84

The general approach looks great to me, but I do think it'd make more sense to pull scoped_test_env env; up to the top of main regardless of _WIN32. Even if that "scope guard" is redundant on Windows, it feels sketchy to declare a "scope guard" variable inside an ifdef when its lifetime extends outside the ifdef.
(If you do this, obviously, do it consistently everywhere.)

mstorsjo added inline comments.Tue, Apr 6, 12:02 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_block_file/is_block_file.pass.cpp
84

Hmm, I can see that it stylistically is weird in one way to have it outlive the ifdef, but I still think I prefer it this way - the ifdef branches set up different variables that then exist for the duration of the surrounding scope... If there's more opinions in this direction I can change it of course.

curdeius added inline comments.Thu, Apr 8, 12:15 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_block_file/is_block_file.pass.cpp
84

I agree with Arthur, it is less surprising to have the guard in all branches of #if. Even if it's not necessary on Windows now, it may become necessary one day, this can avoid stupid mistakes then.

mstorsjo added inline comments.Thu, Apr 8, 12:25 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_block_file/is_block_file.pass.cpp
84

Ok, thanks for chiming in. Updating then.

mstorsjo updated this revision to Diff 336195.Thu, Apr 8, 12:26 PM

Moved the scoped_test_env outside of the ifdef.

ldionne accepted this revision.Fri, Apr 9, 9:29 AM
This revision is now accepted and ready to land.Fri, Apr 9, 9:29 AM