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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added a comment.

I may be repeating myself, but my biggest remark is that there are no tests. I know there's no libcxx CI on Windows, but still, that will make reviewing easier, looking what are the new test cases and what has been forgotten.



================
Comment at: libcxx/src/filesystem/operations.cpp:202-205
+        PosPtr TkStart = consumeRootName(SepEnd, REnd);
+        if (TkStart == REnd)
+          return makeState(PS_InRootDir, RStart, RStart + 1);
         return makeState(PS_InTrailingSep, SepEnd + 1, RStart + 1);
----------------
This looks strange. IIUC it means that you consumed a separator, and then, if `TkStart != nulltptr`, you consumed drive (e.g. "C:").
Can it happen anywhere? `/C:`?

Apart from that, TkStart returned by `consumeRootName` isn't used after the if (in `makeState`).
Shouldn't it be used instead of `SepEnd + 1` (if non null)?


================
Comment at: libcxx/src/filesystem/operations.cpp:207-209
+        PosPtr TkStart = consumeRootName(RStart, REnd);
+        if (TkStart == REnd)
+          return makeState(PS_InRootName, TkStart + 1, RStart + 1);
----------------
You don't check for `TkStart` not being null.


================
Comment at: libcxx/src/filesystem/operations.cpp:210
+          return makeState(PS_InRootName, TkStart + 1, RStart + 1);
+        TkStart = consumeName(RStart, REnd);
         return makeState(PS_InFilenames, TkStart + 1, RStart + 1);
----------------
Why `RStart`? You should use `TkStart` (+ 1?) if it's non-null.


================
Comment at: libcxx/src/filesystem/operations.cpp:222
+      PosPtr TkStart = consumeRootName(SepEnd ? SepEnd : RStart, REnd);
+      if (TkStart == REnd) {
+        if (SepEnd)
----------------
Pretty much the same remarks as above. No null check of `TkStart`.
Then, `SepEnd` is used instead of `TkStart`.


================
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;
----------------
As below, I don't really like allowing P==nullptr.
Shouldn't it be an assertion and callers should ensure it's non-null?


================
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;
----------------
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.


================
Comment at: libcxx/src/filesystem/operations.cpp:363
+    PosPtr Start = P;
+    if (P == nullptr || P == End || isSeparator(*P))
       return nullptr;
----------------
I'm not a big fan of allowing P being nullptr.


================
Comment at: libcxx/src/filesystem/operations.cpp:397-401
+    if (P < End) {
+      return consumeName(consumeSeparators(P, End, 2), End);
+    } else {
+      return consumeSeparators(consumeName(P, End), End, 2);
+    }
----------------
Nit: unnecessary braces around one-line bodies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91176