Page MenuHomePhabricator

[24/N] [libcxx] Make generic_*string return paths with forward slashes on windows
ClosedPublic

Authored by mstorsjo on Nov 10 2020, 8:56 AM.

Details

Summary

This matches what MS STL returns; in std::filesystem, forward slashes is considered a generic dir separator that is valid on all platforms.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 8:56 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Nov 10 2020, 8:56 AM
mstorsjo updated this revision to Diff 308897.Dec 2 2020, 12:33 AM

Updated to use _VSTD:: instead of std::.

Can you @curdeius have a look at this one?

curdeius added inline comments.Dec 18 2020, 3:15 AM
libcxx/include/filesystem
1252

Hmmm. Why can't it be implemented in terms of generic_string?
I.e.: { return generic_string<typename __u8_string::value_type>(); }

1253–1254

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

mstorsjo added inline comments.Dec 18 2020, 3:28 AM
libcxx/include/filesystem
1252

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.

1253–1254

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

curdeius accepted this revision.Dec 18 2020, 4:22 AM

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

libcxx/include/filesystem
1252

Indeed. Thanks for explanation.

1253–1254

OK. Maybe adding a TODO note, or creating a bugzilla ticket for this, would at least keep trace of this. WDYT?

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

Thanks for reviewing!

libcxx/include/filesystem
1253–1254

I can add a note in a comment acknowledging this - I think that's the suitable amount of bureacracy for the issue.

mstorsjo updated this revision to Diff 312759.Dec 18 2020, 4:42 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, added a comment pointing out suboptimal cases of iterating over the strings twice.

mstorsjo updated this revision to Diff 320111.Jan 29 2021, 5:16 AM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, rerunning CI.

mstorsjo updated this revision to Diff 324214.Feb 16 2021, 11:49 PM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Retrigger CI

ldionne accepted this revision.Feb 19 2021, 11:21 AM
This revision is now accepted and ready to land.Feb 19 2021, 11:21 AM