This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix path.decompose for windows
ClosedPublic

Authored by mstorsjo on Oct 22 2020, 3:34 AM.

Details

Summary

Add ifdefs to the test reference tables for cases where paths are interpreted differently (paths that contain a root name).

Fix test assumptions regarding has_root_name() and is_absolute() and add logic to verify the results of is_absolute() for the test cases in the table.

Also add a testcase for the path "//net/", which seemed like an omission.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 22 2020, 3:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 3:34 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Oct 22 2020, 3:34 AM
mstorsjo updated this revision to Diff 326346.Feb 25 2021, 3:56 AM
mstorsjo edited the summary of this revision. (Show Details)
mstorsjo added reviewers: amccarth, curdeius.
curdeius added inline comments.Feb 25 2021, 4:14 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
187

root_name here can possibly have less than 2 characters. You should probably assert that if it's non-empty, then it should have size() >= 2.

mstorsjo added inline comments.Feb 25 2021, 4:22 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
187

Good point, will do that.

Technically, we're not facing unknown inputs here, we only ever get things based on inputs in the table above (and whatever a more or less broken implementation returns based on it), but it's certainly good to verify this in any case.

mstorsjo updated this revision to Diff 326355.Feb 25 2021, 4:23 AM
mstorsjo marked an inline comment as done.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Check that root name is >= 2 chars before inspecting both chars.

mstorsjo added inline comments.Feb 25 2021, 4:24 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
191

Also I know one could write this condition like assert(p.is_absolute() == (root_name[0] == '/' && root_name[1] == '/'));, but I find such code less readable.

curdeius added inline comments.Feb 25 2021, 4:33 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
146

On a second thought... Instead of all this stuff, shouldn't we just add bool is_absolute to PathDecomposeTestcase?

187

Technically, we're not facing unknown inputs here, we only ever get things based on inputs in the table above (and whatever a more or less broken implementation returns based on it), but it's certainly good to verify this in any case.

100% agree.

mstorsjo added inline comments.Feb 25 2021, 4:41 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
146

I guess we could, but that would require both touching all lines in the table to add another field. That's maybe doable, but worse, we'd need to have that field vary in more cases. E.g. /foo/bar is an absolute path on posix, but not on windows (as it lacks a drive name, the path is not considered absolute). So it would essentially require ifdeffing/duplicating the whole table instead of just some sections of it.

curdeius accepted this revision.Feb 25 2021, 4:43 AM

LGTM.

libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
146

Oh, that's a pity. I hadn't thought about the cases outside of the offers.

Quuxplusone added inline comments.Mar 4 2021, 3:08 PM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
97

Conspicuously missing here: "//net/". Any thoughts on adding it as part of this patch?

(LGTM modulo I don't actually get what's going on here. Is it that this test is currently red on Windows and you're just adding the correct test code to make it green on Windows? You don't have to remove any XFAILs or anything as part of this patch?)

mstorsjo updated this revision to Diff 328431.Mar 5 2021, 1:12 AM
mstorsjo edited the summary of this revision. (Show Details)

Added a testcase for //net/.

mstorsjo marked an inline comment as done.Mar 5 2021, 1:31 AM
mstorsjo added inline comments.
libcxx/test/std/input.output/filesystems/class.path/path.member/path.decompose/path.decompose.pass.cpp
97

Good point, I updated the patch including that.

The testing situation in Windows is that I think there used to be a buildbot some years ago, but it is no longer there, and things had regressed a fair bit meanwhile (with at least two different issues that caused 99% of tests to fail). That's all done and fixed now, so right now only around 87 or so tests fail (out of 6.5k). If someone would be willing to set up a buildbot for it (my contributions here are as a voluntary opensource developer only), we could add XFAIL on the remaining bit and track it from there.

Then secondly, these tests for std::filesystem never worked on windows at all. Nowadays the implementation of std::filesystem is working fine on windows (modulo an issue with __int128 in MSVC configurations, see D91139 for discussion around that), but the tests aren't quite there yet.

So for both of those reasons, there's no XFAIL here to remove, but yeah, this patch brings the number of failing tests down from 45 (out of 136) to 44. I've got a series of around 18 patches that fixes all of them (out of which a couple are up for review so far), I'm putting up more of them for review as the current ones get reviewed.

Quuxplusone accepted this revision.Mar 5 2021, 6:58 AM

SGTM, ship it!

This revision is now accepted and ready to land.Mar 5 2021, 6:58 AM
This revision was automatically updated to reflect the committed changes.
mstorsjo marked an inline comment as done.