Page MenuHomePhabricator

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

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

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 1:44 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rnk resigned from this revision.Nov 10 2020, 12:27 PM
mstorsjo updated this revision to Diff 305079.Nov 13 2020, 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
397–398

Just a suggestion to give those magic values some context.

1134

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.Nov 20 2020, 12:05 AM
libcxx/src/filesystem/operations.cpp
397–398

Good point, will apply that.

1134

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.Nov 20 2020, 11:18 AM

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

mstorsjo updated this revision to Diff 312721.Dec 18 2020, 1:36 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, retriggering CI. This one is already formally approved by @amccarth.

ldionne accepted this revision.Jan 13 2021, 8:25 AM
This revision is now accepted and ready to land.Jan 13 2021, 8:25 AM
mstorsjo updated this revision to Diff 317571.Jan 19 2021, 8:18 AM

Rebased on top of updated patch (already preapproved, no need for new review).

mstorsjo updated this revision to Diff 317572.Jan 19 2021, 8:18 AM

Retrigger CI.

mstorsjo updated this revision to Diff 320065.Jan 29 2021, 12:37 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, rerunning CI before pushing