[libcxx-commits] [PATCH] D91176: [20/N] [libcxx] Implement parsing of root_name for paths on windows

[Adrian McCarthy via Phabricator via libcxx-commits]

amccarth added a comment.

This looks good to me, in that I believe it will correctly parse typical native Windows path names.  Only tests will tell, but I understand that there are many patches necessary to enable the tests.

As for the libcxx style issues, I'll leave that to other reviewers.

I'd like to see a name change to clear up the `consumeSeparators` confusion, but I won't block on that.



================
Comment at: libcxx/src/filesystem/operations.cpp:338
   PosPtr consumeSeparator(PosPtr P, PosPtr End) const noexcept {
-    if (P == End || !isSeparator(*P))
+    if (P == nullptr || P == End || !isSeparator(*P))
       return nullptr;
----------------
curdeius wrote:
> As below, I don't really like allowing P==nullptr.
> Shouldn't it be an assertion and callers should ensure it's non-null?
I wouldn't object to an assertion if passing a nullptr is actually a bug, but it's not clear to me that it is.  Since this is a private method, I'm not very concerned about a libcxx user making a bad call here--the risk is contained.


================
Comment at: libcxx/src/filesystem/operations.cpp:347-359
+  PosPtr consumeSeparators(PosPtr P, PosPtr End, int N) const noexcept {
+    PosPtr Ret = consumeSeparator(P, End);
+    if (Ret == nullptr)
+      return nullptr;
+    if (P < End) {
+      if (Ret == P + N)
+        return Ret;
----------------
curdeius wrote:
> mstorsjo wrote:
> > curdeius wrote:
> > > This function, when called with N==2 will consume e.g. `//`, `\\`, but also `/x`, and `\x`.
> > > Is it intended?
> > > I was rather expecting a loop calling `consumeSeparator` up to `N` times.
> > The existing function `consumeSeparator` actually is a bit misnamed, it consumes one or more separators. This function checks that it consumed exactly `N` separators, otherwise we return null and don't consume anything.
> > 
> > Should this maybe be called `consumeExactlyNSeparators` for clarity? (That name is a bit clumsy though.)
> Or just `consumeNSeparators`.
> And then rename `consumeSeparator` to sth like `consumeAllSeparators`.
> Maybe?
Perhaps the existing function should be `consumeSeparators` (plural) and this one should be `consumeNSeparators`.  Fixing the name of the existing method makes this one's implementation more obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91176