This is an archive of the discontinued LLVM Phabricator instance.

[22/N] [libcxx] Implement append and operator/ properly for windows
ClosedPublic

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

Details

Summary

The root_path function has to be changed to return the parsed bit as-is; otherwise a path like //net gets a root path of //net/, as the root name, //net, gets the root directory (an empty string) appended, forming //net/. (The same doesn't happen for the root dir c: though.)

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 8:47 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo edited the summary of this revision. (Show Details)Nov 13 2020, 3:49 AM
mstorsjo added a reviewer: amccarth.
amccarth requested changes to this revision.Dec 11 2020, 11:41 AM
amccarth added inline comments.
libcxx/include/filesystem
1033

The above looks like a literal translation of the standard, so I'd expected it to be platform agnostic. What makes it Windows-specific?

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp
73

I expected 'C:foo`. Using PathAppend, I get C:\ (absolute, not relative).

It's interesting (to me) that the <filesystem> rules don't give either of those answers.

[This is just an observation. Nothing to do here.]

95

The three UNC examples above exactly match those in the #ifdef _WIN32 branch, so perhaps they should be hoisted above the ifdef.

This revision now requires changes to proceed.Dec 11 2020, 11:41 AM
mstorsjo added inline comments.Dec 11 2020, 11:47 AM
libcxx/include/filesystem
1033

It's fairly close to the reference yes. It's not very windows specific in itself, but libc++'s current posix-only implementation is much more straightforward due to having less complicated rules (where there's no root name concept at all).

libcxx/test/std/input.output/filesystems/class.path/path.member/path.append.pass.cpp
95

Sure, I guess they can be hoisted.

mstorsjo updated this revision to Diff 311309.Dec 11 2020, 1:41 PM

Avoided duplicating the testcases that produce identical results on windows and posix.

Ping @amccarth - this requires your follow-up

amccarth accepted this revision.Jan 8 2021, 11:44 AM

LGTM.

ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 28 2021, 3:20 PM
mstorsjo updated this revision to Diff 320110.Jan 29 2021, 5:13 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.

Rerunning CI.

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

Retrigger CI

ldionne requested changes to this revision.Feb 17 2021, 12:39 PM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/filesystem
1033

Would this implementation work on Posix as well? Looking at the Posix implementation, it doesn't appear to be much simpler. Is it faster somehow?

If that's a viable alternative (i.e. it's correct and similarly fast), I would rather simply replace the implementation by yours and have it work on both Posix and Windows.

1079

If we end up keeping those functions separate in the Posix and non-Posix cases, we could get rid of static bool __source_is_absolute(_ECharT) above by instead doing:

bool __source_is_absolute = __is_separator(_Traits::__first_or_null(__src));
if (__source_is_absolute)
  // ...

I feel like having a separate function for __source_is_absolute() isn't worth the trouble anymore if we need to add #ifdefs around it & al.

This revision now requires changes to proceed.Feb 17 2021, 12:39 PM
mstorsjo added inline comments.Feb 17 2021, 1:08 PM
libcxx/include/filesystem
1033

I think it could work, overall - this one is pretty much straight out of the spec for what it should do.

However, this implementation uses an intermediate path object within the three methods below (to handle all the less trivial charset conversion cases when the native one is utf16), which fails tests that force those to not use any extra allocations. (The windows version does some extra allocations, but I think MS STL does the same.)

1079

Ok, I can do that.

We strictly wouldn't need ifdefs around that function, it's just not used in the windows ifdef case, but I agree we can inline it here.

mstorsjo added inline comments.Feb 17 2021, 1:10 PM
libcxx/include/filesystem
1092

This case of __source_is_absolute() is trickier - I think? If we break it out into a separate bool __source_is_absolute, it'd be invoked even if __first == __last. (I'm not sure if that's ok or not.) Or we could just go without the extra intermediate variable there?

mstorsjo updated this revision to Diff 324412.Feb 17 2021, 1:22 PM

Removed the separate __source_is_absolute

mstorsjo updated this revision to Diff 324414.Feb 17 2021, 1:23 PM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Retrigger CI

ldionne accepted this revision.Feb 17 2021, 2:15 PM

LGTM pending CI.

libcxx/include/filesystem
1033

Ehhh I see. I tried finding ways to de-duplicate this in my head but I can't come to something satisfactory. Let's go with that.

This revision is now accepted and ready to land.Feb 17 2021, 2:15 PM