Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The exact-head wcsncmp test compares full UTF-16 code units within the requested bound and places the final span at a protected page; no blocking issue was found.
What I ran
Reviewed PR head 17ad89d3eed470dc4eecee1c98e5ab58ddeb9095 against current main 58d5af7daa8087d2ffbdb50ae6e0db3cb6a88e48; the merge preview was clean (tree 0fa42df0549b5a7ae97552c9e4a5da559f56a604). Environment: Debian 12 x86-64, Python 3.11.2, Git 2.39.5, GCC 12.2.0. I did not build or run C++ tests locally. The author reports the test passed on Linux and Windows as part of a 20/20 integration run, with an exact-head mutation control; those runs were not independently repeated. Hosted policy, report, and portability checks passed; Linux was skipped and build jobs remained pending at final refresh.
git diff --check 58d5af7daa8087d2ffbdb50ae6e0db3cb6a88e48...17ad89d3eed470dc4eecee1c98e5ab58ddeb9095
python3 tools/check_conventions.py --base 58d5af7daa8087d2ffbdb50ae6e0db3cb6a88e48 --head 17ad89d3eed470dc4eecee1c98e5ab58ddeb9095 --title 'test(libc): bound UTF-16 string comparisons at protected pages' --body /tmp/pr-13314-body.md
python3 tools/pr_test_quality.py --root /tmp/anyps5-review-13314 --json core/libs/prx/libc/CMakeLists.txt core/libs/tests/GuestWcsncmpBoundary.cpp
git merge-tree --write-tree 58d5af7daa8087d2ffbdb50ae6e0db3cb6a88e48 refs/review/pr-13314
gh pr checks 13314 -R boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff whitespace | Passed; no output from git diff --check. |
| Conventions and PR template | Passed against current main. |
| Current-main merge preview | Passed; clean tree 0fa42df0549b5a7ae97552c9e4a5da559f56a604. |
| Test registration/source review | Registered as guest_wcsncmp_boundary; cases cover bounds 0–128, first/middle/last differences, both operand orders, equal spans, early full-unit NULs, and unchanged input buffers. |
| Test-quality heuristic | Flagged possible missing registration; manual CMake inspection confirmed registration through ${LIBS_INCLUDE_DIR}/tests/GuestWcsncmpBoundary.cpp. |
| Author-reported tests | Focused test and filtered 20-topic CTest set: 20/20 on Linux and Windows; intentional production mutation rejected by the source-unit control. Not independently run here. |
| Hosted CI | Conventions, eligibility, intake, report, and library portability passed; Linux skipped; label and build jobs pending at final refresh. |
| Build/local C++ execution | Not run locally. |
Unedited local command output
git diff --check produced no output.
Conventions:
All conventions checks passed.
Test-quality heuristic:
{
"findings": [
{
"path": "core/libs/tests/GuestWcsncmpBoundary.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}Merge preview:
0fa42df0549b5a7ae97552c9e4a5da559f56a604
Hosted checks:
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070598570
register-context (macos-15-intel, Debug) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597202
library-portability pass 1m21s https://github.com/boykopovar/AnyPS5/actions/runs/41056220726/job/124064304196
report pass 33s https://github.com/boykopovar/AnyPS5/actions/runs/41056220796/job/124064305191
eligibility pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124064304606
conventions pass 16s https://github.com/boykopovar/AnyPS5/actions/runs/41056220215/job/124064304733
labels pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220222/job/124064305854
intake pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41056220161/job/124064305728
register-context (macos-15-intel, Release) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597208
register-context (ubuntu-latest, Debug) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597173
register-context (ubuntu-latest, Release) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597228
relinker (macos-latest, clang++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597181
relinker (macos-latest, clang++, x86_64) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597475
relinker (ubuntu-latest, g++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597223
relinker (windows-latest, g++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597265
relinker-asan pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597190
windows pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597172
write-watch (macos-15) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597160
write-watch (macos-15-intel) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41056220709/job/124070597304
Negative control
| Assertion | Mutation reported by author | Result |
|---|---|---|
| Test fails when the production comparison is intentionally changed | Author reports an exact-head source-unit mutation | Author reports expected test failure; I did not run this control locally. |
Findings
Blocking
None found in the reviewed source.
Non-blocking
The PR body cites FreeBSD wmemchr(3) as the behavior reference for wcsncmp; that is a different interface. Consider replacing the link with a reference for the tested function. This does not block the test change.
Sources
- Registered
guest_wcsncmp_boundarytarget. - Guarded comparison cases.
- Production
wcsncmp_nid_postfiximplementation. - PR body’s FreeBSD
wmemchr(3)reference.
Not verified
No local build or C++ execution, independent Linux/Windows execution, macOS or WASM runtime, title, or console test. Author-reported integration results were not independently reproduced; hosted Linux skipped and build jobs remained pending.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@MarcZX10 I clarified the reference label and added a direct line anchor for wcsncmp. I checked the exact pinned FreeBSD source through the repository API: wmemchr.3 is the shared wide-character family manual, and its NAME/SYNOPSIS include the tested function(s). The description now makes that explicit rather than relying on the filename. Function declaration. This is a PR-description clarification only. Head AI-assisted: yes. |
|
Investigated the failed macOS checks on unchanged head Both logs stop in The existing fix is #13220 at @Gonic0 these test PRs need the existing registration fix first; thank you for the focused fix. AI-assisted: yes. |
|
Integrated the existing shared CI fix at new head #13220's original commit Fresh local verification:
Hosted-check snapshot at 2026-10-30T17:36:08+00:00 (UTC): 2 pending. Current-head checks. Earlier red checks belong to heads without the prerequisite; new-head CI remains pending until it completes. Please re-check this integrated head. AI-assisted: yes. |
|
Follow-up to the dependency cleanup: GitHub still reported a conflict, so the source branch is now refreshed at The fixture is byte-identical to the preceding published head and the complete combined tree. Combined Description conventions, whitespace, exact main/prerequisite ancestry and fixture retention pass. This is focused native validation, not a full default product, local macOS, PS5/title or GPU run. Please recheck this updated head. Its new hosted checks remain pending until complete. Current-head checks. AI-assisted: yes. |
What
Adds bounded wcsncmp cases through 128 char16_t units, including spans without a terminator inside the limit, low-byte-zero nonzero units, early full-unit NUL, first differences, both operand orders and preservation.
#13220 merged at 2026-11-01T03:58:51Z. Its original commit
e9f09f54fabb6408d3e2711b8df10c10112ab00eremains an ancestor; this independent topic now has no open prerequisite. The earlier shared write-watch configuration fix is already on main and is not a separate feature of this topic.Refreshed the source branch after GitHub still reported a conflict following the dependency cleanup. The topic fixture is unchanged.
Tested
Exact updated head
28612a2accea1e660242b3f85478ed3fdeef81cfintegrates current recorded mainba627802e2210187d417deb64c8f2fed8e33011f. Complete combined validation52f04e5a6b07cea04982202ab1d9237163391686now contains all 31 exact topic heads (including the 17 already refreshed). Windows/MinGW and Linux rebuild all affected targets and their real production library dependencies: all 34 registered CTests pass per OS, zero skips, includingguest_wcsncmp_boundaryexactly once and the three related string groups.The fixture is byte-identical to the preceding published head and the combined tree. No assertion or timeout is weakened. Current-main conventions with this full description, whitespace, exact ancestry and fixture retention pass. This is focused native validation, not a complete default product, local macOS, PS5/title or GPU run. Hosted checks for this new head remain pending until complete.
Earlier rationale and validation (historical revisions and dependency state)
What
Adds bounded wcsncmp cases through 128 char16_t units, including spans without a terminator inside the limit, low-byte-zero nonzero units, early full-unit NUL, first differences, both operand orders and preservation.
Integrate the existing #13220 fix at
e9f09f54fabb6408d3e2711b8df10c10112ab00eand current recorded main. The failed macOS write-watch jobs stopped in CMake configuration because execute-fault and read-fault were each registered twice. This head includes the original prerequisite commit, removing the duplicate block while retaining all 18 distinct tests. No parallel fix is introduced, no test assertion is weakened, and no timeout is changed.This PR's fixture is byte-identical to the previous published head
17ad89d3eed470dc4eecee1c98e5ab58ddeb9095. The extra topic-relative CI change is the existing prerequisite's five-line registration deletion. Proposed landing order is #13220 first, then this independent topic. The submitted head already includes the fix, so its new CI can test it while the prerequisite PR remains open.Original topic rationale and earlier overlap evidence (historical snapshots)
Adds bounded wcsncmp cases through 128 char16_t units, including spans without a terminator inside the limit, low-byte-zero nonzero units, early full-unit NUL, first differences, both operand orders and preservation.
Implementation basis: Public string/memory contract: FreeBSD wide-character family manual, including wcsncmp. Despite the source filename wmemchr.3, its NAME and SYNOPSIS explicitly document these wide-character interfaces; it is a shared family page, not only the wmemchr function. The fixture calls the actual production function(s) through the existing public Api.hpp and constructs expected results separately from immutable input snapshots, membership tables or standard C++ range algorithms. No production function body or firmware object is copied into the fixture.
Known limits: Checks code-unit ordering and comparison sign through the existing declaration. It is not bytewise memcmp, Unicode collation or a firmware ABI measurement.
Source/schema: core/libs/prx/libc/src/Strings.cpp core/libs/prx/libc/include/Api.hpp
Exact topic head
17ad89d3eed470dc4eecee1c98e5ab58ddeb9095is one independent commit on main51c31ea17e9f66cb36fe9655ec31c81610bfcfc9. The current-main repository overlap checker against51c31ea17e9f66cb36fe9655ec31c81610bfcfc9reported no confirmed overlap. It could not finish comparisons for these older PRs already conflicting with main: #5021, #6085, #7081, #7447, #7715, #8150, #8965, #11628, #11661, #11689, #11698, #11947, #12688, #12700, #12965. Their shared-file diffs were reviewed separately. 13 scoped file changes projected onto main and then combined with this topic without a new conflict; 2 peer file changes already conflict with main and are not claimed automatically mergeable. They do not provide this command or test matrix. Registration uses a separate target/name and there is no landing-order dependency. Changes to shared registration before landing need a refreshed merge/test check. Functional overlap remains independent; the shared macOS CI prerequisite is #13220. The twenty current topic heads are also merged into tested integrationcedab5157a32822332671c90812c6780d05157e8.Shared CI prerequisite: #13220 fixes main's duplicate write-watch fault-test registration. The macOS write-watch jobs currently fail in CMake configuration before compiling or executing this PR's fixture. This PR needs that existing fix first for the shared macOS checks; no duplicate implementation or unrelated source change is added here.
Tested
Exact submitted head
1c8549945e9657645da7ce5a98693d50e85b5685. Recorded main6a3afa4deb18ea58a90efb8040fff8c60e4f2aab; prerequisite #13220e9f09f54fabb6408d3e2711b8df10c10112ab00e. Both and the previous published topic head are ancestors. The fixture and topic assertions remain unchanged.Fresh native validation uses combined commit
099d1af76d85818308412d7dd864f8bb4a1cd013, containing all 13 exact submitted heads, on Windows 11 / MinGW GCC 15.2 / CMake 4.3.1 and non-root Ubuntu 26.04 / WSL / GCC 15.2 / CMake 4.2.3. This is explicit combined-commit validation, not 13 separate native product builds. Every individual fixture blob matches that combined tree; pairwise main/prerequisite merges are clean.guest_wcsncmp_boundaryis included exactly once. No timeout was raised.The combined native target build is not the complete default product build. No local macOS/WASM, PS5/title/GPU or firmware validation is claimed. Old red CI belongs to older heads without the prerequisite; new-head hosted CI starts after publication and remains pending until it actually completes. The prerequisite's external branch is unchanged.
Exact composition of the fresh native validation
#13310
ab8eeea15ea0dcb8aabe95e656a041963f8e4e8a, #133131ec676984f08d1ee65496d0d1c487a9fecb8ec49, #133141c8549945e9657645da7ce5a98693d50e85b5685, #13317442aef391ffaec2c9cfe1d098c84dde5f19cd292, #13319dab492c88b6bd8093c351b1726db02ad91af39ab, #13320f27454e88a02dd9c9f0510d3afb0aa72378f7e87, #133788bb9ed3e152ea1611fe8216f25dd03c7830f2818, #133799b39858ef369da4f3eb01e4137e3f932a478ebd2, #133806083413b06d03a34e498629da942b8ec40ab7a9f, #13381cf303f4142403e370f807813b99fc5aa087ad72b, #1339115fbf68aa97b2617cd03ae94f033020028edeac6, #13394cb3188c2b1f36700f882311191f86b897c4f01cb, #133958165ef480879e5eed57eb8f631abc924aed02ec7Earlier author-reported validation (historical heads; not new results)
Topic head:
17ad89d3eed470dc4eecee1c98e5ab58ddeb9095. Combined integration:cedab5157a32822332671c90812c6780d05157e8, based on main51c31ea17e9f66cb36fe9655ec31c81610bfcfc9. Ubuntu/WSL GCC 15.2, CMake 4.2.3, Python 3.14.4; Windows MinGW GCC 15.2, CMake 4.3.1, Python 3.14.3.The registered test calls the actual libc through canonical headers, using OS-protected pages and real input/output buffers. A separate exact-head control compiled the whole relevant production source unit with this fixture: unchanged production code passed, and the fixture rejected an intentional production-function defect. This is a source-unit control, separately from the product build below.
Registered CTest
guest_wcsncmp_boundarypassed on Linux and Windows as part of the filtered twenty-topic run:ctest --test-dir <build> -R <20-topic-pattern> -j 8 -V --output-on-failure, 20/20 passed on each platform, zero skips.cmake --build <linux-full-build> --parallel 8: complete default Linux product build passed. Windows used the full configuration and built the sixteen affected libc test targets plus relinker with--parallel 8; a complete default Windows build is not claimed. Conventions, whitespace, script registration and the public-header declaration/umbrella audit passed. No PS5, title or GPU validation run is claimed. macOS, WASM execution and hosted full-build CI are not reported as passed.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)