[libcxx-commits] [PATCH] D91181: [24/N] [libcxx] Make generic_*string return paths with forward slashes on windows

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM. I'd like just to see a TODO note or a ticket for this double traversal.



================
Comment at: libcxx/include/filesystem:1252
   _VSTD::u32string generic_u32string() const { return generic_string<char32_t>(); }
-  __u8_string generic_u8string() const { return u8string(); }
+  __u8_string generic_u8string() const {
+    __u8_string __s = u8string();
----------------
mstorsjo wrote:
> curdeius wrote:
> > Hmmm. Why can't it be implemented in terms of `generic_string`?
> > I.e.: `{ return generic_string<typename __u8_string::value_type>(); }`
> Because before C++20 (before `char8_t`), there's no typename that specifically triggers UTF8 output; if requesting output as `char`, it's produced in the current windows narrow code page, which in most cases isn't UTF8.
Indeed. Thanks for explanation.


================
Comment at: libcxx/include/filesystem:1253-1254
+  __u8_string generic_u8string() const {
+    __u8_string __s = u8string();
+    _VSTD::replace(__s.begin(), __s.end(), '\\', '/');
+    return __s;
----------------
mstorsjo wrote:
> curdeius wrote:
> > If I'm not mistaken it means that you traverse the string twice, once in `u8string()` to convert it using `_CVT`, and then in `replace()`.
> > I don't think it's a blocker, but just saying.
> > Same above in `generic_string<_ECharT, _Traits, _Allocator>`.
> Yep, that's a correct observation. Suboptimal, but I wouldn't want to complicate things even further (especially at this point of the patchset) for that cause - at this point, getting it to work (at all) and correctly is the only priority...
OK. Maybe adding a TODO note, or creating a bugzilla ticket for this, would at least keep trace of this. WDYT?


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

https://reviews.llvm.org/D91181