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.)
amccarth EricWF ldionne
- Group Reviewers
- rG78d693faecf9: [libcxx] Implement append and operator/ properly for windows
The above looks like a literal translation of the standard, so I'd expected it to be platform agnostic. What makes it Windows-specific?
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.]
The three UNC examples above exactly match those in the #ifdef _WIN32 branch, so perhaps they should be hoisted above the ifdef.
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).
Sure, I guess they can be hoisted.
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.
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.
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.)
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.
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?