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

Fri Dec 18 04:39:21 PST 2020

mstorsjo added a comment.

In D91181#2462705 <https://reviews.llvm.org/D91181#2462705>, @curdeius wrote:

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

Thanks for reviewing!

================
Comment at: libcxx/include/filesystem:1253-1254
+  __u8_string generic_u8string() const {
+    __u8_string __s = u8string();
+    _VSTD::replace(__s.begin(), __s.end(), '\\', '/');
+    return __s;
----------------
curdeius wrote:
> 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?
I can add a note in a comment acknowledging this - I think that's the suitable amount of bureacracy for the issue.

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

https://reviews.llvm.org/D91181