[libcxx-commits] [PATCH] D91176: [20/N] [libcxx] Implement parsing of root_name for paths on windows
Marek Kurdej via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 8 07:00:56 PST 2021
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
More information about the libcxx-commits
mailing list