[libcxx-commits] [PATCH] D89945: [libcxx] [test] Use separate referenes for windows in the path.append test

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added reviewers: curdeius, zoecarver, Mordante, Quuxplusone.
mstorsjo added a comment.

Adding more reviewers on this one too. (Contrary to the other patches, this one doesn't have one earlier LGTM though - but it's been updated according to what was discussed with Louis earlier.)



================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp:112
     }
-    assert(PathEq(LHS, E));
+    assert(LHS == E);
   }
----------------
mstorsjo wrote:
> ldionne wrote:
> > I'm not sure I understand, can you give me an example of why this fails on Windows with `PathEq`?
> `PathEq` checks that the strings are literally identical, while `path::operator==` checks that the paths mean the same (i.e. tolerating both slash forms on windows).
> 
> When we test appending `p2` to `p1` on windows, we get the path `p1\p2`, but the test reference is `p1/p2`.
> 
> We can't just run `make_preferred()` on the reference and do a literal comparison either: If the original path is `p1/p2` and we append `p3`, we get `p1/p2\p3`.
> 
> So this weakens the test a bit (it doesn't distinguish between `p1/p2` and `p1//p2` iirc either), but is a simple change to make. The alternative would be to ifdef the whole table and add duplicate references (or to add `ref_posix`, `ref_windows` in each table entry). Thinking of it now (3 months later), the last option actually sounds pretty good although it makes the patch messier.
To add some context here - I discussed this one with Louis outside of Phabricator back after this review, and we agreed that adding separate references for the expected result for both cases is the best way to go.


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

https://reviews.llvm.org/D89945