Page MenuHomePhabricator

[10/N] [libcxx] Implement _FilesystemClock::now() and __last_write_time for windows
Needs ReviewPublic

Authored by mstorsjo on Tue, Nov 10, 1:44 AM.

Details

Reviewers
amccarth
EricWF
rnk
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Nov 10, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 10, 1:44 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rnk resigned from this revision.Tue, Nov 10, 12:27 PM
mstorsjo updated this revision to Diff 305079.Fri, Nov 13, 3:41 AM

Rebased, taking changes in the preceding patches into account.

This looks good. I'm just confused by one point and have a small suggested edit.

libcxx/src/filesystem/operations.cpp
399

Just a suggestion to give those magic values some context.

1227

It's not clear to me why this checks new_time == file_time_type::min(). It looks like convert_to_timespec returns false if the new time is not representable. In what case would new_time be min? And why would we then say the value is too large?

mstorsjo added inline comments.Fri, Nov 20, 12:05 AM
libcxx/src/filesystem/operations.cpp
399

Good point, will apply that.

1227

This is mostly a workaround to get this test working, when the filesystem time type isn't int128_t: https://github.com/llvm/llvm-project/blob/master/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp#L507-L540

But that test also relies on the test being able to guess what time values are reprepsentable and what aren't, and that doesn't really match what the implementation does anyway, so I guess I could just skip that testcase for now as well, and remove this odd hack here.

And saying the value is "too large" mostly means "out of range" I guess.

amccarth accepted this revision.Fri, Nov 20, 11:18 AM

LGTM. I'll leave it to your discretion on the new_time range check.