Page MenuHomePhabricator

[libcxx] [test] Fix fs.op.last_write_time for Windows
ClosedPublic

Authored by mstorsjo on May 2 2021, 2:22 PM.

Details

Summary

Don't use stat and lstat on Windows; lstat is missing, stat only provides
the modification times with second granularity (and does the wrong thing
regarding symlinks). Instead do a minimal reimplementation using the
native windows APIs.

This is an alternative to D98641. This one contains more compat churn
at the top of the file (reimplememtations of stat and lstat), but
requires much less modifications to the rest of the test. Therefore
I'd kind of prefer this one out of the two.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.May 2 2021, 2:22 PM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2021, 2:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

In general looks good. It would be great if we can get this working without the compat namespace. This would make maintenance slightly easier. Not a blocker since the CI will fail when making a mistake.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
39–40

Is the stat header required on Windows? If not would it be possible to include this header conditional and remove the compat namespace?

92

Please move this line outside the #if section

99

Please remove this line.

mstorsjo added inline comments.May 9 2021, 9:25 AM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
39–40

Ok, that seems to work. I'd generally consider that (trying to avoid that a specific gets included) a bit more brittle, but as long as we have CI, I guess it's fine.

The actual implementation uses a namespace like this though, but as long as we can do without one here, the test remains simpler.

92

If we'd keep the namespace, I wouldn't want to move the ending brace of the namespace outside of the ifdef as long as the starting brace is within the ifdef, that'd be awfully imbalanced. But we can get rid of it altogether.

mstorsjo updated this revision to Diff 343915.May 9 2021, 9:28 AM

Removed the compat namespace, simplifying the patch.

Mordante accepted this revision as: Mordante.May 10 2021, 9:22 AM

It seems the build failed since the patch couldn't be applied. Does this patch depend on other patches?
LGTM, but please make sure it passes CI before committing.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
92

I thought the namespace started before the initial #if. Maybe I misread. But with this version of the patch the comment no longer applies.

It seems the build failed since the patch couldn't be applied. Does this patch depend on other patches?

No, but there's other lines changed closeby by another recently applied patch, regarding the XFAIL lines - rebasing past that and reposting for CI.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
92

No, it started within the #if. But the question is moot in this revision anyway.

mstorsjo updated this revision to Diff 344088.May 10 2021, 9:29 AM

Rebased, rerunning CI.

zoecarver accepted this revision.May 12 2021, 11:37 AM
This revision is now accepted and ready to land.May 12 2021, 11:37 AM
This revision was automatically updated to reflect the committed changes.