[libcxx-commits] [PATCH] D98109: [libcxx] Make path::absolute return paths with separators in preferred form

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D98109#2610569 <https://reviews.llvm.org/D98109#2610569>, @curdeius wrote:

> I already wanted to hit the big green button when I have got a second thought. http://eel.is/c++draft/fs.op.absolute doesn't seem, to my reading, to mandate returning a path with preferred separators. I agree that `current_path` returns a path in native format, but I don't see why`absolute` should force preferred separators. A user passing a path with non-native separators to this function may well know what they are doing and not be willing to pay the cost of transforming separators to the preferred form, however slight this cost might be. What's your opinion on this?

Yes, there's certainly a pretty big gray zone here with respect to how they should behave.

One other reference that we have is how the MS STL behaves - but it's of course not normative in any way. In this case, I had fixed up libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.absolute/absolute.pass.cpp to produce strict matches with MS STL already earlier, but if we don't add this change, we could change the testcase, e.g. like this:

  --- a/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.absolute/absolute.pass.cpp
  +++ b/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.absolute/absolute.pass.cpp
  @@ -42,8 +42,8 @@ TEST_CASE(basic_test)
       } TestCases [] = {
           {"", cwd / ""},
           {"foo", cwd / "foo"},
  -        {"foo/", cwd / "foo" / ""},
  -        {"/already_absolute", cwd.root_path() / "already_absolute"}
  +        {"foo/", cwd / "foo/"},
  +        {"/already_absolute", cwd.root_name() / "/already_absolute"}
       };

That folds the forward slash into the output so that it's kept as is for those cases, and produces strict matches with our current implementation in libc++, but then would make our testcase fail when run against MS STL. (There's plenty of cases where our tests either disagree with MS STL or where our tests are overly specific anyway, so that's not a strict criterion, but having tests that pass with both libc++ and MS STL IMO.)

A third alternative would be to make the absolute.pass.cpp test use separator-agnostic comparisons (i.e. `path::operator==`, while it currently uses `PathEq()` which does `a.native() == b.native()`), which loosens the test a bit.

Out of these options, I felt that making the absolute function return paths with preferred separators was the most sensible choice. Although, one shouldn't be modifying the implementation just for ease of testing.

> On a different note, I think I'll have a closer look at some of the FS tests... The synopses in tests are hum, weird e.g. https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.absolute/absolute.pass.cpp#L13.

Yes, there's for sure things that can be cleaned up in these tests. I'm trying to avoid being distracted with other things for now (other than things that are requested during review), as there's still a bit to go until there's zero failing tests in my setups. :-)

> Also, you maybe know, why do we test only non-throwing version of `absolute`, cf. https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.absolute/absolute.pass.cpp#L50? I believe it's the case for some other functions too.

I don't think there's any intent behind it, only accidental omissions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98109/new/

https://reviews.llvm.org/D98109