[libcxx] [test] Use separate referenes for windows in the path.append test

[ldionne]

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

147

I'm not sure I understand, can you give me an example of why this fails on Windows with PathEq?

[mstorsjo]

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

147

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.

[mstorsjo]

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.)

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

147

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.

[ldionne]

LGTM if we get rid of the expect macro.

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

55–56

Let's use a function for this:

MultiStringType const& expected_result(AppendOperatorTestcase const& tc) {
#if defined(_WIN32)
  return tc. expect_windows;
#else
  return tc.expect_posix;
#endif
};

Then, use expected_result(TC) below instead of TC.expect. WDYT? Feel free to make it a member function if you prefer.

[mstorsjo]

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

55–56

That sounds good, thanks, will do. I'll go with a member function in this case, not that it really matters.

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

 #include "test_iterators.h"
 #include "count_new.h"
 #include "filesystem_test_helper.h"
 
 
 struct AppendOperatorTestcase {
   MultiStringType lhs;
   MultiStringType rhs;
-  MultiStringType expect;
+  MultiStringType expect_posix;
+  MultiStringType expect_windows;
+
+  MultiStringType const& expected_result() const {
+#ifdef _WIN32
+    return expect_windows;
+#else
+    return expect_posix;
+#endif
+  }
 };
 
 #define S(Str) MKSTR(Str)
 const AppendOperatorTestcase Cases[] =
     {
-        {S(""),     S(""),      S("")}
-      , {S("p1"),   S("p2"),    S("p1/p2")}
-      , {S("p1/"),  S("p2"),    S("p1/p2")}
-      , {S("p1"),   S("/p2"),   S("/p2")}
-      , {S("p1/"),  S("/p2"),   S("/p2")}
-      , {S("p1"),   S("\\p2"),  S("p1/\\p2")}
-      , {S("p1\\"), S("p2"),  S("p1\\/p2")}
-      , {S("p1\\"), S("\\p2"),  S("p1\\/\\p2")}
-      , {S(""),     S("p2"),    S("p2")}
-      , {S("/p1"),  S("p2"),    S("/p1/p2")}
-      , {S("/p1"),  S("/p2"),    S("/p2")}
-      , {S("/p1/p3"),  S("p2"),    S("/p1/p3/p2")}
-      , {S("/p1/p3/"),  S("p2"),    S("/p1/p3/p2")}
-      , {S("/p1/"),  S("p2"),    S("/p1/p2")}
-      , {S("/p1/p3/"),  S("/p2/p4"),    S("/p2/p4")}
-      , {S("/"),    S(""),      S("/")}
-      , {S("/p1"), S("/p2/"), S("/p2/")}
-      , {S("p1"),   S(""),      S("p1/")}
-      , {S("p1/"),  S(""),      S("p1/")}
-
-      , {S("//host"),  S("foo"),      S("//host/foo")}
-      , {S("//host/"), S("foo"),      S("//host/foo")}
-      , {S("//host"),  S(""),         S("//host/")}
-
-#ifdef _WIN32
-      , {S("foo"),     S("C:/bar"),   S("C:/bar")}
-      , {S("foo"),     S("C:"),       S("C:")}
-
-      , {S("C:"),      S(""),         S("C:")}
-      , {S("C:foo"),   S("/bar"),     S("C:/bar")}
-      , {S("C:foo"),   S("bar"),      S("C:foo/bar")}
-      , {S("C:/foo"),  S("bar"),      S("C:/foo/bar")}
-      , {S("C:/foo"),  S("/bar"),     S("C:/bar")}
-
-      , {S("C:foo"),   S("C:/bar"),   S("C:/bar")}
-      , {S("C:foo"),   S("C:bar"),    S("C:foo/bar")}
-      , {S("C:/foo"),  S("C:/bar"),   S("C:/bar")}
-      , {S("C:/foo"),  S("C:bar"),    S("C:/foo/bar")}
-
-      , {S("C:foo"),   S("c:/bar"),   S("c:/bar")}
-      , {S("C:foo"),   S("c:bar"),    S("c:bar")}
-      , {S("C:/foo"),  S("c:/bar"),   S("c:/bar")}
-      , {S("C:/foo"),  S("c:bar"),    S("c:bar")}
-
-      , {S("C:/foo"),  S("D:bar"),    S("D:bar")}
-#else
-      , {S("foo"),     S("C:/bar"),   S("foo/C:/bar")}
-      , {S("foo"),     S("C:"),       S("foo/C:")}
-
-      , {S("C:"),      S(""),         S("C:/")}
-      , {S("C:foo"),   S("/bar"),     S("/bar")}
-      , {S("C:foo"),   S("bar"),      S("C:foo/bar")}
-      , {S("C:/foo"),  S("bar"),      S("C:/foo/bar")}
-      , {S("C:/foo"),  S("/bar"),     S("/bar")}
-
-      , {S("C:foo"),   S("C:/bar"),   S("C:foo/C:/bar")}
-      , {S("C:foo"),   S("C:bar"),    S("C:foo/C:bar")}
-      , {S("C:/foo"),  S("C:/bar"),   S("C:/foo/C:/bar")}
-      , {S("C:/foo"),  S("C:bar"),    S("C:/foo/C:bar")}
-
-      , {S("C:foo"),   S("c:/bar"),   S("C:foo/c:/bar")}
-      , {S("C:foo"),   S("c:bar"),    S("C:foo/c:bar")}
-      , {S("C:/foo"),  S("c:/bar"),   S("C:/foo/c:/bar")}
-      , {S("C:/foo"),  S("c:bar"),    S("C:/foo/c:bar")}
+        {S(""),        S(""),         S(""),              S("")}
+      , {S("p1"),      S("p2"),       S("p1/p2"),         S("p1\\p2")}
+      , {S("p1/"),     S("p2"),       S("p1/p2"),         S("p1/p2")}
+      , {S("p1"),      S("/p2"),      S("/p2"),           S("/p2")}
+      , {S("p1/"),     S("/p2"),      S("/p2"),           S("/p2")}
+      , {S("p1"),      S("\\p2"),     S("p1/\\p2"),       S("\\p2")}
+      , {S("p1\\"),    S("p2"),       S("p1\\/p2"),       S("p1\\p2")}
+      , {S("p1\\"),    S("\\p2"),     S("p1\\/\\p2"),     S("\\p2")}
+      , {S(""),        S("p2"),       S("p2"),            S("p2")}
+      , {S("/p1"),     S("p2"),       S("/p1/p2"),        S("/p1\\p2")}
+      , {S("/p1"),     S("/p2"),      S("/p2"),           S("/p2")}
+      , {S("/p1/p3"),  S("p2"),       S("/p1/p3/p2"),     S("/p1/p3\\p2")}
+      , {S("/p1/p3/"), S("p2"),       S("/p1/p3/p2"),     S("/p1/p3/p2")}
+      , {S("/p1/"),    S("p2"),       S("/p1/p2"),        S("/p1/p2")}
+      , {S("/p1/p3/"), S("/p2/p4"),   S("/p2/p4"),        S("/p2/p4")}
+      , {S("/"),       S(""),         S("/"),             S("/")}
+      , {S("/p1"),     S("/p2/"),     S("/p2/"),          S("/p2/")}
+      , {S("p1"),      S(""),         S("p1/"),           S("p1\\")}
+      , {S("p1/"),     S(""),         S("p1/"),           S("p1/")}
+
+      , {S("//host"),  S("foo"),      S("//host/foo"),    S("//host\\foo")}
+      , {S("//host/"), S("foo"),      S("//host/foo"),    S("//host/foo")}
+      , {S("//host"),  S(""),         S("//host/"),       S("//host\\")}
+
+      , {S("foo"),     S("C:/bar"),   S("foo/C:/bar"),    S("C:/bar")}
+      , {S("foo"),     S("C:"),       S("foo/C:"),        S("C:")}
+
+      , {S("C:"),      S(""),         S("C:/"),           S("C:")}
+      , {S("C:foo"),   S("/bar"),     S("/bar"),          S("C:/bar")}
+      , {S("C:foo"),   S("bar"),      S("C:foo/bar"),     S("C:foo\\bar")}
+      , {S("C:/foo"),  S("bar"),      S("C:/foo/bar"),    S("C:/foo\\bar")}
+      , {S("C:/foo"),  S("/bar"),     S("/bar"),          S("C:/bar")}
+
+      , {S("C:foo"),   S("C:/bar"),   S("C:foo/C:/bar"),  S("C:/bar")}
+      , {S("C:foo"),   S("C:bar"),    S("C:foo/C:bar"),   S("C:foo\\bar")}
+      , {S("C:/foo"),  S("C:/bar"),   S("C:/foo/C:/bar"), S("C:/bar")}
+      , {S("C:/foo"),  S("C:bar"),    S("C:/foo/C:bar"),  S("C:/foo\\bar")}
+
+      , {S("C:foo"),   S("c:/bar"),   S("C:foo/c:/bar"),  S("c:/bar")}
+      , {S("C:foo"),   S("c:bar"),    S("C:foo/c:bar"),   S("c:bar")}
+      , {S("C:/foo"),  S("c:/bar"),   S("C:/foo/c:/bar"), S("c:/bar")}
+      , {S("C:/foo"),  S("c:bar"),    S("C:/foo/c:bar"),  S("c:bar")}
 
-      , {S("C:/foo"),  S("D:bar"),    S("C:/foo/D:bar")}
-#endif
+      , {S("C:/foo"),  S("D:bar"),    S("C:/foo/D:bar"),  S("D:bar")}
     };
 
 
 const AppendOperatorTestcase LongLHSCases[] =
     {
-        {S("p1"),   S("p2"),    S("p1/p2")}
-      , {S("p1/"),  S("p2"),    S("p1/p2")}
-      , {S("p1"),   S("/p2"),   S("/p2")}
-      , {S("/p1"),  S("p2"),    S("/p1/p2")}
+        {S("p1"),   S("p2"),    S("p1/p2"),  S("p1\\p2")}
+      , {S("p1/"),  S("p2"),    S("p1/p2"),  S("p1/p2")}
+      , {S("p1"),   S("/p2"),   S("/p2"),    S("/p2")}
+      , {S("/p1"),  S("p2"),    S("/p1/p2"), S("/p1\\p2")}
     };
 #undef S
 
 
 // The append operator may need to allocate a temporary buffer before a code_cvt
 // conversion. Test if this allocation occurs by:
 //   1. Create a path, `LHS`, and reserve enough space to append `RHS`.
 //      This prevents `LHS` from allocating during the actual appending.
 //   2. Create a `Source` object `RHS`, which represents a "large" string.
 //      (The string must not trigger the SSO)
 //   3. Append `RHS` to `LHS` and check for the expected allocation behavior.
 template <class CharT>
 void doAppendSourceAllocTest(AppendOperatorTestcase const& TC)
 {
   using namespace fs;
   using Ptr = CharT const*;
   using Str = std::basic_string<CharT>;
   using StrView = std::basic_string_view<CharT>;
   using InputIter = input_iterator<Ptr>;
 
   const Ptr L = TC.lhs;
   Str RShort = (Ptr)TC.rhs;
-  Str EShort = (Ptr)TC.expect;
+  Str EShort = (Ptr)TC.expected_result();
   assert(RShort.size() >= 2);
   CharT c = RShort.back();
   RShort.append(100, c);
   EShort.append(100, c);
   const Ptr R = RShort.data();
   const Str& E = EShort;
   std::size_t ReserveSize = E.size() + 3;
   // basic_string
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     Str  RHS(R);
     {
       DisableAllocationGuard g;
       LHS /= RHS;
     }
     assert(PathEq(LHS, E));
   }
   // basic_string_view
   {
     path LHS(L); PathReserve(LHS, ReserveSize);
     StrView  RHS(R);
     {
       DisableAllocationGuard g;
       LHS /= RHS;

 {
   using namespace fs;
   using Ptr = CharT const*;
   using Str = std::basic_string<CharT>;
   using StrView = std::basic_string_view<CharT>;
   using InputIter = input_iterator<Ptr>;
   const Ptr L = TC.lhs;
   const Ptr R = TC.rhs;
-  const Ptr E = TC.expect;
+  const Ptr E = TC.expected_result();
   // basic_string
   {
     path Result(L);
     Str RHS(R);
     path& Ref = (Result /= RHS);
     assert(Result == E);
     assert(&Ref == &Result);
   }
int main(int, char**)
   using namespace fs;
   for (auto const & TC : Cases) {
     {
       const char* LHS_In = TC.lhs;
       const char* RHS_In = TC.rhs;
       path LHS(LHS_In);
       path RHS(RHS_In);
       path& Res = (LHS /= RHS);
-      assert(PathEq(Res, (const char*)TC.expect));
+      assert(PathEq(Res, (const char*)TC.expected_result()));
       assert(&Res == &LHS);
     }
     doAppendSourceTest<char>    (TC);
     doAppendSourceTest<wchar_t> (TC);
     doAppendSourceTest<char16_t>(TC);
     doAppendSourceTest<char32_t>(TC);
   }
   for (auto const & TC : LongLHSCases) {
     (void)TC;
     LIBCPP_ONLY(doAppendSourceAllocTest<char>(TC));
     LIBCPP_ONLY(doAppendSourceAllocTest<wchar_t>(TC));
   }
   test_sfinae();
 
   return 0;
 }