This is an archive of the discontinued LLVM Phabricator instance.

[6/N] [libcxx] Handle backslash as path separator on windows
ClosedPublic

Authored by mstorsjo on Nov 10 2020, 1:35 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 1:35 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
amccarth added inline comments.Dec 1 2020, 2:56 PM
libcxx/include/filesystem
1122
  1. It looks like most places are using preferred_separator, so is there a ever case where you'd have slashes instead of backslashes?
  1. If I've read correctly, the Windows version always uses wide characters, so maybe the character literals should be L'/' and L'\\' so that they naturally match the value type of the iterator.
mstorsjo added inline comments.Dec 1 2020, 11:10 PM
libcxx/include/filesystem
1122
  1. Yes, the thing is that when a user constructs a path from a string that itself contains path separators, forward slashes are always considered a path separator (as platform agnostic separator), and backslashes work as separator too, on windows only. The filesystem::path internal representation of a path keeps them in whatever form they were input, but this modifier converts the path to the preferred (native) form. FWIW this is exactly what MS STL's version of make_preferred() does too.
  2. Sure, that'd work. For these cases, the plain char form doesn't produce any warnings, but I can change it to wide char literals just for clarity.
mstorsjo updated this revision to Diff 308878.Dec 1 2020, 11:17 PM

Use wide char literals for forward and back slashes within an windows ifdef.

mstorsjo marked an inline comment as done.Dec 1 2020, 11:17 PM
mstorsjo updated this revision to Diff 308896.Dec 2 2020, 12:31 AM

Updated to use _VSTD::replace instead of std::replace in the installed header.

curdeius added inline comments.
libcxx/include/filesystem
1122

Not sure it will be easier to read, but why not adding (always defined) __posix_preferred_separator and __windows_preferred_separator and using them here?

libcxx/src/filesystem/operations.cpp
53

No need for static in an anonymous namespace.

mstorsjo added inline comments.Dec 10 2020, 12:18 AM
libcxx/include/filesystem
1122

Hmm, maybe - although I'm not sure if it really helps with readability?

libcxx/src/filesystem/operations.cpp
53

Thanks, will fix.

mstorsjo updated this revision to Diff 310797.Dec 10 2020, 1:29 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Removed the unnecessary static in an anonymous namespace.

@ldionne This one is also an early dependency of the rest of the patchset; it doesn't yet have a formal approval of any others, but seems to be mostly ok, with just a small style preference question left (whether to explicitly use '\\' and '/' literals, or unconditionally add something like __posix_preferred_separator and __windows_preferred_separator).

mstorsjo updated this revision to Diff 311809.Dec 14 2020, 11:42 PM
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

mstorsjo updated this revision to Diff 312439.Dec 17 2020, 3:53 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

Can either @amccarth or @curdeius mark this as formally approved, if there's no other strict blockers for it, to maybe help with getting it looked at?

No modifications in tests?

No modifications in tests?

It's covered by existing tests, more or less, I haven't singled out which specific bit covers this.

The thing is, currently, libc++'s tests (in general, not specific to std::filesystem) on windows are in a severely broken state (with around >100 tests failing out of the box). And libc++'s tests for std::filesystem assume lots of posix-specific behaviours. I have a huge branch with 50+ patches that make libc++'s tests pass with MS STL (as I'd consider the reference for how std::filesystem should behave on windows), and in that state of the testsuite, all those same tests pass with libc++ when this whole patchset is applied.

And it's hard to single out specific test changes among the patches in the beginning of this patchset, as you need up to around patch 17 or 18 before libc++ with filesystem enabled actually compiles successfully for windows.

I tried reverting this particular patch on top of the otherwise fully patched branch with all tests passing, and that broke 80 out of 137 testcases - so I think it's fairly covered. I think all bits in the patch has been written to make some aspect of the existing testset work.

curdeius accepted this revision.Dec 18 2020, 3:55 AM

Fair enough. LGTM.

ldionne accepted this revision.Jan 6 2021, 2:56 PM
This revision is now accepted and ready to land.Jan 6 2021, 2:56 PM
This revision was automatically updated to reflect the committed changes.