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

[mstorsjo]

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.

[curdeius]

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

186

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]

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

186

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]

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

[mstorsjo]

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

190

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]

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?

186

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]

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]

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]

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]

Added a testcase for //net/.

[mstorsjo]

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]

SGTM, ship it!

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

const PathDecomposeTestcase PathTestCases[] =
     , {"..", {".."}, "", "", "", "..", "", ".."}
     , {"foo", {"foo"}, "", "", "", "foo", "", "foo"}
     , {"/", {"/"}, "/", "", "/", "", "/", ""}
     , {"/foo", {"/", "foo"}, "/", "", "/", "foo", "/", "foo"}
     , {"foo/", {"foo", ""}, "", "", "", "foo/", "foo", ""}
     , {"/foo/", {"/", "foo", ""}, "/", "", "/", "foo/", "/foo", ""}
     , {"foo/bar", {"foo","bar"}, "",  "", "",  "foo/bar", "foo", "bar"}
     , {"/foo//bar", {"/","foo","bar"}, "/", "", "/", "foo/bar", "/foo", "bar"}
+#ifdef _WIN32
+    , {"//net", {"//net"}, "//net", "//net", "", "", "//net", ""}
+    , {"//net/", {"//net", "/"}, "//net/", "//net", "/", "", "//net/", ""}
+    , {"//net/foo", {"//net", "/", "foo"}, "//net/", "//net", "/", "foo", "//net/", "foo"}
+#else
     , {"//net", {"/", "net"}, "/", "", "/", "net", "/", "net"}
+    , {"//net/", {"/", "net", ""}, "/", "", "/", "net/", "//net", ""}
     , {"//net/foo", {"/", "net", "foo"}, "/", "", "/", "net/foo", "/net", "foo"}
+#endif
     , {"///foo///", {"/", "foo", ""}, "/", "", "/", "foo///", "///foo", ""}
     , {"///foo///bar", {"/", "foo", "bar"}, "/", "", "/", "foo///bar", "///foo", "bar"}
     , {"/.", {"/", "."}, "/", "", "/", ".", "/", "."}
     , {"./", {".", ""}, "", "", "", "./", ".", ""}
     , {"/..", {"/", ".."}, "/", "", "/", "..", "/", ".."}
     , {"../", {"..", ""}, "", "", "", "../", "..", ""}
     , {"foo/.", {"foo", "."}, "", "", "", "foo/.", "foo", "."}
     , {"foo/..", {"foo", ".."}, "", "", "", "foo/..", "foo", ".."}
     , {"foo/./", {"foo", ".", ""}, "", "", "", "foo/./", "foo/.", ""}
     , {"foo/./bar", {"foo", ".", "bar"}, "", "", "", "foo/./bar", "foo/.", "bar"}
     , {"foo/../", {"foo", "..", ""}, "", "", "", "foo/../", "foo/..", ""}
     , {"foo/../bar", {"foo", "..", "bar"}, "", "", "", "foo/../bar", "foo/..", "bar"}
+#ifdef _WIN32
+    , {"c:", {"c:"}, "c:", "c:", "", "", "c:", ""}
+    , {"c:/", {"c:", "/"}, "c:/", "c:", "/", "", "c:/", ""}
+    , {"c:foo", {"c:", "foo"}, "c:", "c:", "", "foo", "c:", "foo"}
+    , {"c:/foo", {"c:", "/", "foo"}, "c:/", "c:", "/", "foo", "c:/", "foo"}
+    , {"c:foo/", {"c:", "foo", ""}, "c:", "c:", "", "foo/", "c:foo", ""}
+    , {"c:/foo/", {"c:", "/", "foo", ""}, "c:/", "c:", "/", "foo/",  "c:/foo", ""}
+    , {"c:/foo/bar", {"c:", "/", "foo", "bar"}, "c:/", "c:", "/", "foo/bar", "c:/foo", "bar"}
+#else
     , {"c:", {"c:"}, "", "", "", "c:", "", "c:"}
     , {"c:/", {"c:", ""}, "", "", "", "c:/", "c:", ""}
     , {"c:foo", {"c:foo"}, "", "", "", "c:foo", "", "c:foo"}
     , {"c:/foo", {"c:", "foo"}, "", "", "", "c:/foo", "c:", "foo"}
     , {"c:foo/", {"c:foo", ""}, "", "", "", "c:foo/", "c:foo", ""}
     , {"c:/foo/", {"c:", "foo", ""}, "", "", "", "c:/foo/",  "c:/foo", ""}
     , {"c:/foo/bar", {"c:", "foo", "bar"}, "", "", "", "c:/foo/bar", "c:/foo", "bar"}
+#endif
     , {"prn:", {"prn:"}, "", "", "", "prn:", "", "prn:"}
+#ifdef _WIN32
+    , {"c:\\", {"c:", "\\"}, "c:\\", "c:", "\\", "", "c:\\", ""}
+    , {"c:\\foo", {"c:", "\\", "foo"}, "c:\\", "c:", "\\", "foo", "c:\\", "foo"}
+    , {"c:foo\\", {"c:", "foo", ""}, "c:", "c:", "", "foo\\", "c:foo", ""}
+    , {"c:\\foo\\", {"c:", "\\", "foo", ""}, "c:\\", "c:", "\\", "foo\\", "c:\\foo", ""}
+    , {"c:\\foo/",  {"c:", "\\", "foo", ""}, "c:\\", "c:", "\\", "foo/", "c:\\foo", ""}
+    , {"c:/foo\\bar", {"c:", "/", "foo", "bar"}, "c:\\", "c:", "\\", "foo\\bar", "c:/foo", "bar"}
+#else
     , {"c:\\", {"c:\\"}, "", "", "", "c:\\", "", "c:\\"}
     , {"c:\\foo", {"c:\\foo"}, "", "", "", "c:\\foo", "", "c:\\foo"}
     , {"c:foo\\", {"c:foo\\"}, "", "", "", "c:foo\\", "", "c:foo\\"}
     , {"c:\\foo\\", {"c:\\foo\\"}, "", "", "", "c:\\foo\\", "", "c:\\foo\\"}
     , {"c:\\foo/",  {"c:\\foo", ""}, "", "", "", "c:\\foo/", "c:\\foo", ""}
     , {"c:/foo\\bar", {"c:", "foo\\bar"}, "", "", "", "c:/foo\\bar", "c:", "foo\\bar"}
+#endif
     , {"//", {"/"}, "/", "", "/", "", "/", ""}
   };
 
 void decompPathTest()
 {
   using namespace fs;
   for (auto const & TC : PathTestCases) {
     fs::path p(TC.raw);
     assert(p == TC.raw);
 
     assert(p.root_path() == TC.root_path);
     assert(p.has_root_path() != TC.root_path.empty());
 
+#ifndef _WIN32
     assert(p.root_name().native().empty());
+#endif
     assert(p.root_name() == TC.root_name);
     assert(p.has_root_name() != TC.root_name.empty());
 
     assert(p.root_directory() == TC.root_directory);
     assert(p.has_root_directory() != TC.root_directory.empty());
 
     assert(p.relative_path() == TC.relative_path);
     assert(p.has_relative_path() != TC.relative_path.empty());
 
     assert(p.parent_path() == TC.parent_path);
     assert(p.has_parent_path() != TC.parent_path.empty());
 
     assert(p.filename() == TC.filename);
     assert(p.has_filename() != TC.filename.empty());
 
+#ifdef _WIN32
+    if (!p.has_root_name()) {
+      assert(p.is_absolute() == false);
+    } else {
+      std::string root_name = p.root_name().string();
+      assert(root_name.length() >= 2);
+      if (root_name[1] == ':') {
+        // Drive letter, absolute if has a root directory
         assert(p.is_absolute() == p.has_root_directory());
+      } else {
+        // Possibly a server path
+        // Convert to one separator style, for simplicity
+        std::replace(root_name.begin(), root_name.end(), '\\', '/');
+        if (root_name[0] == '/' && root_name[1] == '/')
+          assert(p.is_absolute() == true);
+        else
+          assert(p.is_absolute() == false);
+      }
+    }
+#else
+    assert(p.is_absolute() == p.has_root_directory());
+#endif
     assert(p.is_relative() != p.is_absolute());
     if (p.empty())
       assert(p.is_relative());
 
     assert(static_cast<std::size_t>(std::distance(p.begin(), p.end())) == TC.elements.size());
     assert(std::equal(p.begin(), p.end(), TC.elements.begin(), ComparePathExact()));
 
     // check backwards