[6/N] [libcxx] Handle backslash as path separator on windows

[amccarth]

libcxx/include/filesystem

1122

1. It looks like most places are using preferred_separator, so is there a ever case where you'd have slashes instead of backslashes?

2. If I've read correctly, the Windows version always uses wide characters, so maybe the character literals should be L'/' and L'\\' so that they naturally match the value type of the iterator.

[mstorsjo]

libcxx/include/filesystem

1122

1. Yes, the thing is that when a user constructs a path from a string that itself contains path separators, forward slashes are always considered a path separator (as platform agnostic separator), and backslashes work as separator too, on windows only. The filesystem::path internal representation of a path keeps them in whatever form they were input, but this modifier converts the path to the preferred (native) form. FWIW this is exactly what MS STL's version of make_preferred() does too.
2. Sure, that'd work. For these cases, the plain char form doesn't produce any warnings, but I can change it to wide char literals just for clarity.

[mstorsjo]

Use wide char literals for forward and back slashes within an windows ifdef.

[mstorsjo]

Updated to use _VSTD::replace instead of std::replace in the installed header.

[curdeius]

libcxx/include/filesystem

1122

Not sure it will be easier to read, but why not adding (always defined) __posix_preferred_separator and __windows_preferred_separator and using them here?

libcxx/src/filesystem/operations.cpp

55

No need for static in an anonymous namespace.

[mstorsjo]

libcxx/include/filesystem

1122

Hmm, maybe - although I'm not sure if it really helps with readability?

libcxx/src/filesystem/operations.cpp

55

Thanks, will fix.

[mstorsjo]

Removed the unnecessary static in an anonymous namespace.

[mstorsjo]

@ldionne This one is also an early dependency of the rest of the patchset; it doesn't yet have a formal approval of any others, but seems to be mostly ok, with just a small style preference question left (whether to explicitly use '\\' and '/' literals, or unconditionally add something like __posix_preferred_separator and __windows_preferred_separator).

[mstorsjo]

Rebased

[mstorsjo]

Rebased

[mstorsjo]

Can either @amccarth or @curdeius mark this as formally approved, if there's no other strict blockers for it, to maybe help with getting it looked at?

[curdeius]

No modifications in tests?

[mstorsjo]

In D91138#2462595, @curdeius wrote:

No modifications in tests?

It's covered by existing tests, more or less, I haven't singled out which specific bit covers this.

The thing is, currently, libc++'s tests (in general, not specific to std::filesystem) on windows are in a severely broken state (with around >100 tests failing out of the box). And libc++'s tests for std::filesystem assume lots of posix-specific behaviours. I have a huge branch with 50+ patches that make libc++'s tests pass with MS STL (as I'd consider the reference for how std::filesystem should behave on windows), and in that state of the testsuite, all those same tests pass with libc++ when this whole patchset is applied.

And it's hard to single out specific test changes among the patches in the beginning of this patchset, as you need up to around patch 17 or 18 before libc++ with filesystem enabled actually compiles successfully for windows.

I tried reverting this particular patch on top of the otherwise fully patched branch with all tests passing, and that broke 80 out of 137 testcases - so I think it's fairly covered. I think all bits in the patch has been written to make some aspect of the existing testset work.

[curdeius]

Fair enough. LGTM.

libcxx/include/filesystem

     _PathCVT<_ItVal>::__append_range(__pn_, __first, __last);
     return *this;
   }
 
   // modifiers
   _LIBCPP_INLINE_VISIBILITY
   void clear() noexcept { __pn_.clear(); }
 
-  path& make_preferred() { return *this; }
+  path& make_preferred() {
+#if defined(_LIBCPP_WIN32API)
+    _VSTD::replace(__pn_.begin(), __pn_.end(), L'/', L'\\');
+#endif
+    return *this;
+  }
 
   _LIBCPP_INLINE_VISIBILITY
   path& remove_filename() {
     auto __fname = __filename();
     if (!__fname.empty())
       __pn_.erase(__fname.data() - __pn_.data());
     return *this;
   }

libcxx/src/filesystem/operations.cpp

 
 #if defined(__ELF__) && defined(_LIBCPP_LINK_RT_LIB)
 # pragma comment(lib, "rt")
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 
 namespace {
+
+static bool isSeparator(path::value_type C) {
+  if (C == '/')
+    return true;
+#if defined(_LIBCPP_WIN32API)
+  if (C == '\\')
+    return true;
+#endif
+  return false;
+}
+
 namespace parser {
 
 using string_view_t = path::__string_view;
 using string_view_pair = pair<string_view_t, string_view_t>;
 using PosPtr = path::value_type const*;
 
 struct PathParser {
   enum ParserState : unsigned char {
case PS_InTrailingSep:
       return &RawEntry.front();
     case PS_AtEnd:
       return &Path.back() + 1;
     }
     _LIBCPP_UNREACHABLE();
   }
 
   PosPtr consumeSeparator(PosPtr P, PosPtr End) const noexcept {
-    if (P == End || *P != '/')
+    if (P == End || !isSeparator(*P))
       return nullptr;
     const int Inc = P < End ? 1 : -1;
     P += Inc;
-    while (P != End && *P == '/')
+    while (P != End && isSeparator(*P))
       P += Inc;
     return P;
   }
 
   PosPtr consumeName(PosPtr P, PosPtr End) const noexcept {
-    if (P == End || *P == '/')
+    if (P == End || isSeparator(*P))
       return nullptr;
     const int Inc = P < End ? 1 : -1;
     P += Inc;
-    while (P != End && *P != '/')
+    while (P != End && !isSeparator(*P))
       P += Inc;
     return P;
   }
 };
 
 string_view_pair separate_filename(string_view_t const& s) {
   if (s == PS(".") || s == PS("..") || s.empty())
     return string_view_pair{s, PS("")};
if (PP.State == PathParser::PS_InRootDir)
     return *PP;
   return {};
 }
 
 string_view_t path::__root_path_raw() const {
   auto PP = PathParser::CreateBegin(__pn_);
   if (PP.State == PathParser::PS_InRootName) {
     auto NextCh = PP.peek();
-    if (NextCh && *NextCh == '/') {
+    if (NextCh && isSeparator(*NextCh)) {
       ++PP;
       return createView(__pn_.data(), &PP.RawEntry.back());
     }
     return PP.RawEntry;
   }
   if (PP.State == PathParser::PS_InRootDir)
     return *PP;
   return {};
static PathPartKind ClassifyPathPart(string_view_t Part) {
   if (Part.empty())
     return PK_TrailingSep;
   if (Part == PS("."))
     return PK_Dot;
   if (Part == PS(".."))
     return PK_DotDot;
   if (Part == PS("/"))
     return PK_RootSep;
+#if defined(_LIBCPP_WIN32API)
+  if (Part == PS("\\"))
+    return PK_RootSep;
+#endif
   return PK_Filename;
 }
 
 path path::lexically_normal() const {
   if (__pn_.empty())
     return *this;
 
   using PartKindPair = pair<string_view_t, PathPartKind>;