[libcxx-commits] [PATCH] D91178: [22/N] [libcxx] Implement append and operator/ properly for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/include/filesystem:1079
     using _CVT = _PathCVT<_SourceChar<_Source> >;
     if (__source_is_absolute(_Traits::__first_or_null(__src)))
       __pn_.clear();
----------------
ldionne wrote:
> 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.
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.


================
Comment at: libcxx/include/filesystem:1033
+    __pn_ += __p.__pn_.substr(__p_root_name_size);
+    return *this;
+  }
----------------
ldionne wrote:
> mstorsjo wrote:
> > amccarth wrote:
> > > The above looks like a literal translation of the standard, so I'd expected it to be platform agnostic.  What makes it Windows-specific?
> > 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).
> 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.
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91178