[libcxx-commits] [PATCH] D64818: [libcxx] Construct path using a instance of std::locale

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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



================
Comment at: libcxx/include/__locale:1367
+        const int __sz = 32;
+        _InternT __buf[__sz];
+        _InternT* __bn;
----------------
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]);


================
Comment at: libcxx/include/__locale:1374
+            __throw_runtime_error("locale not supported");
+        for (const _InternT* __p = __buf; __p < __bn; ++__p, ++__s)
+            *__s = (wchar_t)*__p;
----------------
`__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.)


================
Comment at: libcxx/include/filesystem:749
+template <class _InputIt, 
+         class = typename enable_if<!is_constructible<const char*, _InputIt>::value>::type>
+inline _LIBCPP_INLINE_VISIBILITY
----------------
`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?


================
Comment at: libcxx/include/filesystem:753
+  const string __s(__b, __e);
+  return __widen_from_char_iter_pair(__s.data(), __s.data() + __s.size(), __loc);
+}
----------------
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...


================
Comment at: libcxx/include/filesystem:792
+    for (; *__e != '\0'; ++__e)
+      ;
+    return __widen_from_char_iter_pair(__b, __e, __loc);
----------------
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);
```


================
Comment at: libcxx/include/filesystem:838
+  using _EnableIfWidenableFromCharSource =
+      typename enable_if<__is_widenable_from_char_source<_SourceOrIter>::value, _Tp>::type;
+  
----------------
```
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.


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.construct/source_and_locale.pass.cpp:32
+template <class ...Args>
+void RunTestCase(const char* TestPath, const char* Expect, std::locale Locale, Args... args) {
+  using namespace fs;
----------------
Should `std::locale` be passed as `const std::locale&` instead? Does it matter? (I don't know.)


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.construct/source_and_locale.pass.cpp:97
+    static_assert(std::__is_input_iterator<It>::value, "");
+    //static_assert(std::is_constructible<path, It, std::locale>::value, "");
+  }
----------------
Why is this commented out?


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

https://reviews.llvm.org/D64818