[libc++] Implement missing filesystem::path constructors with locale

[ldionne]

Summary

This patch provides the two missing constructors for std::filesystem::path
using an instance of std::locale to convert the source.

Given that the source needs to be converted using the
codecvt<wchar_t, char, mbstate_t> facet of the std::locale at first,
these two constructors:

1. Can only take a char source.
2. Need to have different path than the current two other constructors taking a source.

After converting to a wchar_t source using the codecvt, we need to convert
a second time to store the result. The standard is unclear on how the
second conversion should happen:

> Otherwise a conversion is performed using the codecvt<wchar_t, char, mbstate_t>
> facet of loc, and then a second conversion to the current ordinary encoding.

I interpreted this as the second conversion can be executed however we want,
meaning that I can reuse the facilities from the two constructors that accept
any kind of source.

[jguegant]

Note: this is my first contribution to libc++. I tried to follow the style used in the rest of files, but I may have missed some hidden coding style rules: some files use four spaces, some two? Which SFINAE pattern (return type, class paramter...) is to be preferred? Free template functions vs template classes with static member functions?

[• Quuxplusone]

libcxx/include/filesystem

809

char{} is a strange way to spell '\0', and since sentinel is used only on line 811, I think you don't need sentinel at all.

[jguegant]

libcxx/include/filesystem

741

Not sure what is the expected behavior if the locale does not this facet.

809

Sure, that make sense now! It used to be a generic CharT.

[jguegant]

Removed the unnecessary variables __sentinel.

[• Quuxplusone]

Please re-upload the diff with full context (e.g. git diff -U999 blah blah).

BTW, I'm giving a mostly-superficial review, and only because we talked on Slack the other day. I have no opinion as to whether this patch is wanted by libc++, no opinion as to whether it's correct from a charset-wonk POV, and no ability to land it. You'll still have to get interest from one of the three listed reviewers in order to make concrete progress here. :)

libcxx/include/__locale

1367

Peanut gallery says: I'm not 100% sure of the rules about const. I observe that this line seems to be okay even in -pedantic mode, but intuitively I would expect __buf to be a VLA. IMHO it would be clearer to make __sz a constexpr int instead of just a const int — or even my generally preferred formulation,

_InternT __buf[32];
constexpr ptrdiff_t __sz = sizeof(__buf) / sizeof(__buf[0]);

1374

__s is of user-provided type. Depending on level of library-maintainer paranoia, you might want to make this ++__p, void(), ++__s, or pull ++__s down into the body of the for-loop (and add curly braces). The current formulation will call a user-defined operator,(const char*, _OutputIterator) if one exists. (And will perform ADL to find out if one exists, regardless.)

libcxx/include/filesystem

749

class = _EnableIf<!is_constructible<const char*, _InputIt>::value>
But also, how is that constructibility relevant? Does the Standard say something like "If the input iterator is explicitly convertible to const char* then do X, else do Y"?

Furthermore, this codepath is going to do heap-allocation with string __s(__b, __e). Can we avoid the heap-allocation somehow?

753

I'm worried about the way you use overloading on the name __widen_from_char_iter_pair. If some well-intentioned maintainer removed the const on line 752, then line 753 would turn into an infinite recursion. I don't see any reason to use overloading here. Maybe change __widen_from_char_iter_pair on lines 739, 753, 766, 776, 793, and 811 to __widen_from_char_pointer_pair? That would leave only line 889 as a potential source of extra heap-allocations...

792

This could probably be more simply expressed as

string_view __sv(__b);
return __widen_from_char_iter_pair(__sv.data(), __sv.data() + __sv.size(), __loc);

838

template <class _SourceOrIter>
using _EnableIfWidenableFromCharSource =
    _EnableIf<__is_widenable_from_char_source<_SourceOrIter>::value>;

You don't seem to use the _Tp parameter for anything at the moment.

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

32

Should std::locale be passed as const std::locale& instead? Does it matter? (I don't know.)

97

Why is this commented out?

[jguegant]

Thanks a lot @Quuxplusone! I am aware that you cannot take any final decision on what gets in but these are really helpful comments here.
I will fix some of you remarks and upload a better diff (with proper context to get a better picture) afterwards.

Any way to gently attract the three reviewers here?

libcxx/include/__locale

1367

This function is a dumb generalisation of the functions 1389, 1426 which had this pattern.

Correct me if I am wrong but this will work as C-arrays have such syntax:

D1 [ constant-expressionopt ] attribute-specifier-seqopt

Where:

The constant-expression shall be a converted constant expression of type std::size_­t ([expr.const]).

Here const int should work thanks to a special rule for const-qualified integrals:

A variable is usable in constant expressions after its initializing declaration is encountered if it is a constexpr variable, or it is of reference type or of const-qualified integral or enumeration type, and its initializer is a constant initializer.

1374

Good idea! Now that this function accept more than one type, it seems a reasonable to be paranoid.

libcxx/include/filesystem

749

The idea here is to give the priority to the overload with const char* which effectively make both char* and const char*being routed by the first overload line 739. Obviously, my intent is not really clear here, I wonder how I could express that in a better way if I continue in that direction. Maybe having 3 overloads? const char*, char* and generic? A more descriptive trait name?

753

I believe that this would not create an infinite recursion, the enable_if is sending both char* and const char* to the first overload line 739.

I am not sure if there is a smart way to avoid allocation here. Maybe if I call codecvt::in on every character from the iterator. But wouldn't that be slow? What happen if your code-point is made of 3-4 char instead of one? Should I then copy X amounts of characters into a buffer allocated on the stack and send process the iterator chunks by chunks?
I have a similar issue line 808 and if anyone has a clever solution, I am all for it!

I had a look at how other constructors for path already in place (like line 681 or 692) do it and they already heap-allocate on even more code-paths:

template <class _Iter>
static void __append_range(string& __dest, _Iter __b, _Iter __e) {
  static_assert(!is_same<_Iter, _ECharT*>::value, "Call const overload");
  if (__b == __e)
    return;
  basic_string<_ECharT> __tmp(__b, __e);
  _Narrower()(back_inserter(__dest), __tmp.data(),
              __tmp.data() + __tmp.length());
}

If we go in your direction, 889 would now heap-allocation in all scenarios, whereas now the 889 overload supplied with const char* and char* would avoid allocating a string.

792

Neat! Will do that.

838

Indeed, I will remove it!

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

32

I was thinking that locale is a pretty cheap type (a sort of reference counted pointer), but I might be wrong. Taking it by reference cannot make it worst ;)

97

This should not be here. Thanks for spotting it!

[jguegant]

Fixed few remarks mentioned by @Quuxplusone.

[ldionne]

[Github PR transition cleanup]

Commandeering to finish.

[ldionne]

Rebase. Simplify the implementation a bit. Remove some tests for SFINAE-friendliness that are not necessary according to my reading of the spec.

[ldionne]

Add missing _LIBCPP_HIDE_FROM_ABI

[ldionne]

Fix C++03 build.

[ldionne]

Make sure we initialize mbstate_t to avoid uninitialized read.

[ldionne]

Try to fix the CI

[ldionne]

Remove invalid test -- we're not allowed to construct a path from a wchar_t* using the locale constructor, only char* it allowed.

[tahonermann]

I think this is looking good. Before I bless it though, I want to make sure the intent is clear.

The new locale sensitive constructors will use the codecvt<wchar_t, char, ...> facet from the specified locale to convert the provided char-based source (locale dependent encoding) to a wchar_t-based representation (UTF-16 or UTF-32 depending on platform). If on Windows, done. If on POSIX, the wchar_t-based representation is then converted to UTF-8 in char-based storage.

That seems like the right semantics; or at least the best that we can do given that filenames don't have strongly associated encodings. I haven't managed to validate for myself yet; is this consistent with the non-locale based constructors that take a char-based source? I presume those are assumed to be UTF-8 on POSIX regardless of whatever the current locale settings are.

If it isn't too difficult to do, it would be good to use a real locale (e.g., one that uses ISO-8859-1 encoding) and validate the conversion to UTF-8 (POSIX) or UTF-16 (Windows) in the test with some characters that require re-encoding. The existing test validates that all the characters are changed to 'o', but doesn't exercise a change to encoded length or that the characters didn't undergo a further conversion (exercising non-ASCII characters would help with that).

[Mordante]

No real issue from my side. I'll leave the approval to Tom.

libcxx/include/__filesystem/path.h

552 ↗

(On Diff #557895)

557 ↗

(On Diff #557895)

[tahonermann]

libcxx/include/__locale

1566–1579

This can end up in an infinite loop when the narrow input string ends in a partial code unit sequence. In that case, partial will be returned, but from_next (aka __nn) will be left pointing to the beginning of the partial sequence such that the next iteration will encounter the same partial result.

See https://godbolt.org/z/3q5xaG5dW for an example. The test there reliably exhibits an infinite loop for std::codecvt<char32_t, char, std::mbstate_t>. For the std::codecvt<wchar_t, char, std::mbstate_t> facet, the infinite loop is avoided because the converter packs partial state into std::mbstate_t and then (erroneously) returns ok instead of partial (erroneously because no character is ever translated, but no error is ever issued either).

These appear to be existing bug given that this code was just factored out from other locations.

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

130

I think additional tests should be added here to exercise various edge cases like the truncated partial code unit sequence in the godbolt link I added in another comment. We should also exercise some MxN sequence conversions (e.g., UTF-8 to UTF-16). That can be done by calling .u16string() on the resulting path object and comparing what it returns to expectations.

libcxx/include/__locale

     }
 };
 
+template<class _InternT, class _ExternT, class _StateT, class _OutputIterator>
+inline _LIBCPP_INLINE_VISIBILITY
+_OutputIterator 
+__widen_using_codecvt(codecvt<_InternT, _ExternT, _StateT> const& __cvt, 
+                             _OutputIterator __s, 
+                             const char* __nb, const char* __ne)
+{
+    codecvt_base::result __r = codecvt_base::ok;
+    mbstate_t __mb;
+    while (__nb < __ne && __r != codecvt_base::error)
+    {
+        const int __sz = 32;
+        _InternT __buf[__sz];
+        _InternT* __bn;
+        const char* __nn = __nb;
+        __r = __cvt.in(__mb, __nb, __ne - __nb > __sz ? __nb+__sz : __ne, __nn,
+                    __buf, __buf+__sz, __bn);
+        if (__r == codecvt_base::error || __nn == __nb)
+            __throw_runtime_error("locale not supported");
+        for (const _InternT* __p = __buf; __p < __bn; ++__p, ++__s)
+            *__s = (wchar_t)*__p;
+        __nb = __nn;
+    }
+    return __s;
+}
+
 template <size_t _Np>
 struct __widen_from_utf8
 {

     _OutputIterator
     operator()(_OutputIterator __s, const char* __nb, const char* __ne) const
     {
-        result __r = ok;
-        mbstate_t __mb;
-        while (__nb < __ne && __r != error)
-        {
-            const int __sz = 32;
-            char16_t __buf[__sz];
-            char16_t* __bn;
-            const char* __nn = __nb;
-            __r = do_in(__mb, __nb, __ne - __nb > __sz ? __nb+__sz : __ne, __nn,
-                        __buf, __buf+__sz, __bn);
-            if (__r == codecvt_base::error || __nn == __nb)
-                __throw_runtime_error("locale not supported");
-            for (const char16_t* __p = __buf; __p < __bn; ++__p, ++__s)
-                *__s = (wchar_t)*__p;
-            __nb = __nn;
-        }
-        return __s;
+        return __widen_using_codecvt(*this, __s, __nb, __ne);
     }
 };
 

     _OutputIterator
     operator()(_OutputIterator __s, const char* __nb, const char* __ne) const
     {
-        result __r = ok;
-        mbstate_t __mb;
-        while (__nb < __ne && __r != error)
-        {
-            const int __sz = 32;
-            char32_t __buf[__sz];
-            char32_t* __bn;
-            const char* __nn = __nb;
-            __r = do_in(__mb, __nb, __ne - __nb > __sz ? __nb+__sz : __ne, __nn,
-                        __buf, __buf+__sz, __bn);
-            if (__r == codecvt_base::error || __nn == __nb)
-                __throw_runtime_error("locale not supported");
-            for (const char32_t* __p = __buf; __p < __bn; ++__p, ++__s)
-                *__s = (wchar_t)*__p;
-            __nb = __nn;
-        }
-        return __s;
+        return __widen_using_codecvt(*this, __s, __nb, __ne);
     }
 };
 

libcxx/include/filesystem

   }
 };
 
+inline _LIBCPP_INLINE_VISIBILITY
+wstring __widen_from_char_iter_pair(const char* __b, const char* __e, const locale& __loc) {
+  wstring __r;
+  if (!has_facet<codecvt<wchar_t, char, mbstate_t>>(__loc))
+    return __r;
+  __r.reserve(__e - __b);
+  __widen_using_codecvt(use_facet<codecvt<wchar_t, char, mbstate_t>>(__loc) , back_inserter(__r), __b, __e);
+  return __r;
+}
+
+template <class _InputIt, 
+         class = typename enable_if<!is_constructible<const char*, _InputIt>::value>::type>
+inline _LIBCPP_INLINE_VISIBILITY
+wstring __widen_from_char_iter_pair(_InputIt __b, _InputIt __e, const locale& __loc) {
+  const string __s(__b, __e);
+  return __widen_from_char_iter_pair(__s.data(), __s.data() + __s.size(), __loc);
+}
+
+template <class _Tp>
+struct __is_widenable_from_char_string : public false_type {};
+
+template <class _Traits, class _Alloc>
+struct __is_widenable_from_char_string<
+    basic_string<char, _Traits, _Alloc>>
+    : true_type {
+  using _Str = basic_string<char, _Traits, _Alloc>; 
+
+  static wstring __convert(_Str const& __s, const locale& __loc) {
+    return __widen_from_char_iter_pair(__s.data(), __s.data() + __s.size(), __loc);
+  }
+};
+
+template <class _Traits>
+struct __is_widenable_from_char_string<
+    basic_string_view<char, _Traits>>
+    : true_type {
+  using _Str = basic_string_view<char, _Traits>; 
+  static wstring __convert(_Str const& __sv, const locale& __loc) {
+    return __widen_from_char_iter_pair(__sv.data(), __sv.data() + __sv.size(), __loc);
+  }
+};
+
+template <class _Source, class _DS = typename decay<_Source>::type,
+          class _UnqualPtrType =
+              typename remove_const<typename remove_pointer<_DS>::type>::type>
+struct __is_widenable_from_char_array : false_type {};
+
+template <class _Source, class _ECharT>
+struct __is_widenable_from_char_array<_Source, _ECharT*, char>
+    : true_type {
+
+  static wstring __convert(const char* __b, const locale& __loc) {
+    const char* __e = __b;
+    for (; *__e != '\0'; ++__e)
+      ;
+    return __widen_from_char_iter_pair(__b, __e, __loc);
+  }
+};
+
+template <class _Iter, bool _IsIt = __is_input_iterator<_Iter>::value,
+          class = char>
+struct __is_widenable_from_char_iter : false_type {};
+
+template <class _Iter>
+struct __is_widenable_from_char_iter <
+    _Iter, true,
+    typename remove_reference<typename iterator_traits<_Iter>::value_type>::type >
+    : true_type {
+
+  static wstring __convert(_Iter __i, const locale& __loc) {
+    string __s;
+    for (; *__i != '\0'; ++__i)
+      __s.push_back(*__i);
+    return __widen_from_char_iter_pair(__s.data(), __s.data() + __s.size(), __loc);
+  }
+};
+
+template <class _Tp, bool _IsStringT = __is_widenable_from_char_string<_Tp>::value,
+          bool IsCharArray = __is_widenable_from_char_array<_Tp>::value,
+          bool _IsIterT = !IsCharArray && __is_widenable_from_char_iter<_Tp>::value>
+struct __is_widenable_from_char_source : false_type {
+  static_assert(!_IsStringT && !IsCharArray && !_IsIterT, "Must all be false");
+};
+
+template <class _Tp>
+struct __is_widenable_from_char_source<_Tp, true, false, false> : __is_widenable_from_char_string<_Tp> {};
+
+template <class _Tp>
+struct __is_widenable_from_char_source<_Tp, false, true, false> : __is_widenable_from_char_array<_Tp> {};
+
+template <class _Tp>
+struct __is_widenable_from_char_source<_Tp, false, false, true> : __is_widenable_from_char_iter<_Tp> {};
+
 class _LIBCPP_TYPE_VIS path {
   template <class _SourceOrIter, class _Tp = path&>
   using _EnableIfPathable =
       typename enable_if<__is_pathable<_SourceOrIter>::value, _Tp>::type;
 
+  template <class _SourceOrIter, class _Tp = path&>
+  using _EnableIfWidenableFromCharSource =
+      typename enable_if<__is_widenable_from_char_source<_SourceOrIter>::value, _Tp>::type;
+  
   template <class _Tp>
   using _SourceChar = typename __is_pathable<_Tp>::__char_type;
 

     _PathCVT<_ItVal>::__append_range(__pn_, __first, __last);
   }
 
-  // TODO Implement locale conversions.
-  template <class _Source, class = _EnableIfPathable<_Source, void> >
-  path(const _Source& __src, const locale& __loc, format = format::auto_format);
+  template <class _Source, class = _EnableIfWidenableFromCharSource<_Source> >
+  path(const _Source& __src, const locale& __loc, 
+       format = format::auto_format) {
+    using _Widener = __is_widenable_from_char_source<_Source>;
+    _SourceCVT<_VSTD::wstring>::__append_source(__pn_, _Widener::__convert(__src, __loc));
+  }
+
   template <class _InputIt>
-  path(_InputIt __first, _InputIt _last, const locale& __loc,
-       format = format::auto_format);
+  path(_InputIt __first, _InputIt __last, const locale& __loc,
+       format = format::auto_format) {
+    _SourceCVT<_VSTD::wstring>::__append_source(__pn_, __widen_from_char_iter_pair(__first, __last, __loc));
+  }
 
   _LIBCPP_INLINE_VISIBILITY
   ~path() = default;

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

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03
+
+// <filesystem>
+
+// class path
+
+// template <class Source>
+//      path(const Source& source);
+// template <class InputIterator>
+//      path(InputIterator first, InputIterator last);
+
+
+#include "filesystem_include.hpp"
+#include <locale>
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+#include "test_iterators.h"
+#include "min_allocator.h"
+#include "filesystem_test_helper.hpp"
+
+template <class ...Args>
+void RunTestCase(const char* TestPath, const char* Expect, std::locale Locale, Args... args) {
+  using namespace fs;
+  const char* TestPathEnd = StrEnd(TestPath);
+  const std::size_t Size = TestPathEnd - TestPath;
+  const std::size_t SSize = StrEnd(Expect) - Expect;
+  assert(Size == SSize);
+  // StringTypes
+  {
+    const std::string S(TestPath);
+    path p(S, Locale, args...); 
+    assert(p.native() == Expect);
+    assert(p.string<char>() == Expect);
+  }
+  {
+    const std::string_view S(TestPath);
+    path p(S, Locale, args...); 
+    assert(p.native() == Expect);
+    assert(p.string<char>() == Expect);
+  }
+  // Char* pointers
+  {
+    path p(TestPath, Locale, args...);
+    assert(p.native() == Expect);
+    assert(p.string<char>() == Expect);
+  }
+  {
+    path p(TestPath, TestPathEnd, Locale, args...);
+    assert(p.native() == Expect);
+    assert(p.string<char>() == Expect);
+  }
+  // Iterators
+  {
+    using It = input_iterator<const char*>;
+    path p(It{TestPath}, Locale, args...);
+    assert(p.native() == Expect);
+    assert(p.string<char>() == Expect);
+  }
+  {
+    using It = input_iterator<const char*>;
+    path p(It{TestPath}, It{TestPathEnd}, Locale, args...);
+    assert(p.native() == Expect);
+    assert(p.string<char>() == Expect);
+  }
+}
+
+void test_sfinae() {
+  using namespace fs;
+  {
+    using It = const char* const;
+    static_assert(std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    using It = input_iterator<const char*>;
+    static_assert(std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    struct Traits {
+      using iterator_category = std::input_iterator_tag;
+      using value_type = const char;
+      using pointer = const char*;
+      using reference = const char&;
+      using difference_type = std::ptrdiff_t;
+    };
+    using It = input_iterator<const char*, Traits>;
+    static_assert(std::__is_input_iterator<It>::value, "");
+    //static_assert(std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    using It = const wchar_t* const;
+    static_assert(!std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    using It = input_iterator<const wchar_t*>;
+    static_assert(!std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    struct Traits {
+      using iterator_category = std::input_iterator_tag;
+      using value_type = const wchar_t;
+      using pointer = const wchar_t*;
+      using reference = const wchar_t&;
+      using difference_type = std::ptrdiff_t;
+    };
+    using It = input_iterator<const wchar_t*, Traits>;
+    static_assert(!std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    using It = output_iterator<const char*>;
+    static_assert(!std::is_constructible<path, It, std::locale>::value, "");
+  }
+  {
+    static_assert(!std::is_constructible<path, int*, std::locale>::value, "");
+  }
+}
+
+struct CustomCodeCvt : std::codecvt<wchar_t, char, std::mbstate_t> {
+protected:
+  result do_in(state_type&,
+               const extern_type* from, const extern_type* from_end, const extern_type*& from_next,
+               intern_type* to, intern_type* to_end, intern_type*& to_next) const override {
+    for (; from < from_end && to < to_end; ++from, ++to)
+      *to = 'o';
+    
+    from_next = from;
+    to_next = to;
+
+    return result::ok;
+  }
+};
+
+int main(int, char**) {
+  std::locale Locale;
+
+  // Ensure std::codecvt<wchar_t, char, std::mbstate_t> is used.
+  std::locale CustomLocale(Locale, new CustomCodeCvt());
+  std::string TestPath("aaaa");
+  std::string Expect("oooo");
+  RunTestCase(TestPath.c_str(), Expect.c_str(), CustomLocale);
+  RunTestCase(TestPath.c_str(), Expect.c_str(), CustomLocale, fs::path::format::auto_format);
+  RunTestCase(TestPath.c_str(), Expect.c_str(), CustomLocale, fs::path::format::native_format);
+  RunTestCase(TestPath.c_str(), Expect.c_str(), CustomLocale, fs::path::format::generic_format);
+
+  for (auto const& MS : PathList) {
+    RunTestCase(MS, MS, Locale);
+    RunTestCase(MS, MS, Locale, fs::path::format::auto_format);
+    RunTestCase(MS, MS, Locale, fs::path::format::native_format);
+    RunTestCase(MS, MS, Locale, fs::path::format::generic_format);
+  }
+
+  test_sfinae();
+
+  return 0;
+}