[22/N] [libcxx] Implement append and operator/ properly for windows

[mstorsjo]

Summary

The root_path function has to be changed to return the parsed bit as-is; otherwise a path like //net gets a root path of //net/, as the root name, //net, gets the root directory (an empty string) appended, forming //net/. (The same doesn't happen for the root dir c: though.)

[amccarth]

libcxx/include/filesystem

1033

The above looks like a literal translation of the standard, so I'd expected it to be platform agnostic. What makes it Windows-specific?

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

75

I expected 'C:foo`. Using PathAppend, I get C:\ (absolute, not relative).

It's interesting (to me) that the <filesystem> rules don't give either of those answers.

[This is just an observation. Nothing to do here.]

97

The three UNC examples above exactly match those in the #ifdef _WIN32 branch, so perhaps they should be hoisted above the ifdef.

[mstorsjo]

libcxx/include/filesystem

1033

It's fairly close to the reference yes. It's not very windows specific in itself, but libc++'s current posix-only implementation is much more straightforward due to having less complicated rules (where there's no root name concept at all).

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

97

Sure, I guess they can be hoisted.

[mstorsjo]

Avoided duplicating the testcases that produce identical results on windows and posix.

[mstorsjo]

Ping @amccarth - this requires your follow-up

[amccarth]

LGTM.

[mstorsjo]

Rerunning CI.

[mstorsjo]

Rebased

[mstorsjo]

Retrigger CI

[ldionne]

libcxx/include/filesystem

1033

Would this implementation work on Posix as well? Looking at the Posix implementation, it doesn't appear to be much simpler. Is it faster somehow?

If that's a viable alternative (i.e. it's correct and similarly fast), I would rather simply replace the implementation by yours and have it work on both Posix and Windows.

1079

If we end up keeping those functions separate in the Posix and non-Posix cases, we could get rid of static bool __source_is_absolute(_ECharT) above by instead doing:

bool __source_is_absolute = __is_separator(_Traits::__first_or_null(__src));
if (__source_is_absolute)
  // ...

I feel like having a separate function for __source_is_absolute() isn't worth the trouble anymore if we need to add #ifdefs around it & al.

[mstorsjo]

libcxx/include/filesystem

1033

I think it could work, overall - this one is pretty much straight out of the spec for what it should do.

However, this implementation uses an intermediate path object within the three methods below (to handle all the less trivial charset conversion cases when the native one is utf16), which fails tests that force those to not use any extra allocations. (The windows version does some extra allocations, but I think MS STL does the same.)

1079

Ok, I can do that.

We strictly wouldn't need ifdefs around that function, it's just not used in the windows ifdef case, but I agree we can inline it here.

[mstorsjo]

libcxx/include/filesystem

1092

This case of __source_is_absolute() is trickier - I think? If we break it out into a separate bool __source_is_absolute, it'd be invoked even if __first == __last. (I'm not sure if that's ok or not.) Or we could just go without the extra intermediate variable there?

[mstorsjo]

Removed the separate __source_is_absolute

[mstorsjo]

Retrigger CI

[ldionne]

LGTM pending CI.

libcxx/include/filesystem

1033

Ehhh I see. I tried finding ways to de-duplicate this in my head but I can't come to something satisfactory. Let's go with that.

libcxx/include/filesystem

   path& assign(_InputIt __first, _InputIt __last) {
     typedef typename iterator_traits<_InputIt>::value_type _ItVal;
     __pn_.clear();
     _PathCVT<_ItVal>::__append_range(__pn_, __first, __last);
     return *this;
   }
 
 private:
+#if !defined(_LIBCPP_WIN32API)
   template <class _ECharT>
   static bool __source_is_absolute(_ECharT __first_or_null) {
     return __is_separator(__first_or_null);
   }
+#endif
 
 public:
   // appends
+#if defined(_LIBCPP_WIN32API)
+  path& operator/=(const path& __p) {
+    auto __p_root_name = __p.__root_name();
+    auto __p_root_name_size = __p_root_name.size();
+    if (__p.is_absolute() ||
+        (!__p_root_name.empty() && __p_root_name != root_name())) {
+      __pn_ = __p.__pn_;
+      return *this;
+    }
+    if (__p.has_root_directory()) {
+      path __root_name_str = root_name();
+      __pn_ = __root_name_str.native();
+      __pn_ += __p.__pn_.substr(__p_root_name_size);
+      return *this;
+    }
+    if (has_filename() || (!has_root_directory() && is_absolute()))
+      __pn_ += preferred_separator;
+    __pn_ += __p.__pn_.substr(__p_root_name_size);
+    return *this;
+  }
+  template <class _Source>
+  _LIBCPP_INLINE_VISIBILITY _EnableIfPathable<_Source>
+  operator/=(const _Source& __src) {
+    return operator/=(path(__src));
+  }
+
+  template <class _Source>
+  _EnableIfPathable<_Source> append(const _Source& __src) {
+    return operator/=(path(__src));
+  }
+
+  template <class _InputIt>
+  path& append(_InputIt __first, _InputIt __last) {
+    return operator/=(path(__first, __last));
+  }
+#else
   path& operator/=(const path& __p) {
     if (__p.is_absolute()) {
       __pn_ = __p.__pn_;
       return *this;
     }
     if (has_filename())
       __pn_ += preferred_separator;
     __pn_ += __p.native();
     return *this;
   }
 
   // FIXME: Use _LIBCPP_DIAGNOSE_WARNING to produce a diagnostic when __src
   // is known at compile time to be "/' since the user almost certainly intended
   // to append a separator instead of overwriting the path with "/"
   template <class _Source>
   _LIBCPP_INLINE_VISIBILITY _EnableIfPathable<_Source>
   operator/=(const _Source& __src) {
     return this->append(__src);
   }
 
   template <class _Source>
   _EnableIfPathable<_Source> append(const _Source& __src) {
     using _Traits = __is_pathable<_Source>;
     using _CVT = _PathCVT<_SourceChar<_Source> >;
     if (__source_is_absolute(_Traits::__first_or_null(__src)))
       __pn_.clear();
     else if (has_filename())
       __pn_ += preferred_separator;
     _CVT::__append_source(__pn_, __src);
     return *this;
   }
 
   template <class _InputIt>
   path& append(_InputIt __first, _InputIt __last) {
     typedef typename iterator_traits<_InputIt>::value_type _ItVal;
     static_assert(__can_convert_char<_ItVal>::value, "Must convertible");
     using _CVT = _PathCVT<_ItVal>;
     if (__first != __last && __source_is_absolute(*__first))
       __pn_.clear();
     else if (has_filename())
       __pn_ += preferred_separator;
     _CVT::__append_range(__pn_, __first, __last);
     return *this;
   }
+#endif
 
   // concatenation
   _LIBCPP_INLINE_VISIBILITY
   path& operator+=(const path& __x) {
     __pn_ += __x.__pn_;
     return *this;
   }
 

   // decomposition
   _LIBCPP_INLINE_VISIBILITY path root_name() const {
     return string_type(__root_name());
   }
   _LIBCPP_INLINE_VISIBILITY path root_directory() const {
     return string_type(__root_directory());
   }
   _LIBCPP_INLINE_VISIBILITY path root_path() const {
+#if defined(_LIBCPP_WIN32API)
+    return string_type(__root_path_raw());
+#else
     return root_name().append(string_type(__root_directory()));
+#endif
   }
   _LIBCPP_INLINE_VISIBILITY path relative_path() const {
     return string_type(__relative_path());
   }
   _LIBCPP_INLINE_VISIBILITY path parent_path() const {
     return string_type(__parent_path());
   }
   _LIBCPP_INLINE_VISIBILITY path filename() const {

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

#endif
       , {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("C:/foo"),  S("D:bar"),    S("C:/foo/D:bar")}
+#endif
     };
 
 
 const AppendOperatorTestcase LongLHSCases[] =
     {
         {S("p1"),   S("p2"),    S("p1/p2")}
       , {S("p1/"),  S("p2"),    S("p1/p2")}
       , {S("p1"),   S("/p2"),   S("/p2")}