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

Wed Feb 17 14:15:59 PST 2021

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM pending CI.

================
Comment at: libcxx/include/filesystem:1033
+    __pn_ += __p.__pn_.substr(__p_root_name_size);
+    return *this;
+  }
----------------
mstorsjo wrote:
> 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.)
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91178