Repository navigation
feat(libSceIcu): implement u_strCompareIter and u_getUnicodeVersion - #13457
adriadam10 merged 1 commit into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The ICU implementation and focused test are internally consistent, but integration order for overlapping open ICU PRs has not been agreed or recorded.
What I ran
Reviewed head d1499225373ff23a47ec40f71fff70dcb0bd46e4 against PR base 9f748f4d8e36ce3714f3b95f3386316e110a9b22. A read-only merge preview onto current main 9304896d89f070f65b8f38bd3a8024d5c19f8fe1 was clean (tree 32a2c10ad0b45aa84831510d0817260601510a7e). Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0.
git diff --check 9f748f4d8e36ce3714f3b95f3386316e110a9b22...d1499225373ff23a47ec40f71fff70dcb0bd46e4
python3 tools/check_conventions.py --base 9f748f4d8e36ce3714f3b95f3386316e110a9b22 --head d1499225373ff23a47ec40f71fff70dcb0bd46e4
python3 tools/pr_abi_audit.py --base 9f748f4d8e36ce3714f3b95f3386316e110a9b22 --head d1499225373ff23a47ec40f71fff70dcb0bd46e4 --json > /tmp/anyps5-pr13457-abi.json
python3 - <<'PY'
import sys
sys.path.insert(0, 'tools')
import nid_names
for name in ('u_strCompareIter', 'u_getUnicodeVersion'):
print(name, nid_names.compute_nid(name))
PY
python3 -c 'import json; d=json.load(open("/tmp/anyps5-pr13457-abi.json")); print("status:", d.get("status")); print("symbols:", len(d.get("symbols", []))); print("findings:", d.get("findings", []))'
python3 tools/pr_test_quality.py --root . core/libs/prx/libSceIcu/tests/CompareIter.cpp
git merge-tree --write-tree refs/remotes/upstream/main refs/remotes/review/pr-13457
python3 tools/pr_precheck.py --base 9f748f4d8e36ce3714f3b95f3386316e110a9b22 --head refs/remotes/review/pr-13457 --no-negative-controls --output-dir /tmp/anyps5-pr13457-precheckResults
| Check | Result |
|---|---|
| Diff check and conventions | Passed |
| ABI audit | Passed with two warnings for removed legacy stub names; replacement names compute to the same NIDs |
| Test-quality heuristic | Warned that it could not find the test path literally; I verified registration in the PR-head CMake file through ${CMAKE_CURRENT_SOURCE_DIR} |
| Current-main merge preview | Clean |
| Aggregate no-build precheck | Its isolated copy lacked local-only, untracked audit scripts; I ran the relevant checks directly from the workspace |
Unedited output from the direct checks
All conventions checks passed.
core/libs/prx/libSceIcu/tests/CompareIter.cpp:1: possible-unregistered-cpp-test: Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage.
Heuristic findings require manual interpretation.
status: passed_with_warnings
symbols: 105
findings: [{'symbol': 'sceIcuUnknown_TNZrG2iXikw_nid_no_patch', 'kind': 'export-removed', 'message': 'This APS5_VABI declaration, definition, or explicit alias appeared in the changed base files but not in the corresponding head files; verify that removing it is intentional.'}, {'symbol': 'sceIcuUnknown_zGuhKwXM9I0_nid_no_patch', 'kind': 'export-removed', 'message': 'This APS5_VABI declaration, definition, or explicit alias appeared in the changed base files but not in the corresponding head files; verify that removing it is intentional.'}]
All conventions checks passed.
u_strCompareIter TNZrG2iXikw
u_getUnicodeVersion zGuhKwXM9I0
git diff --check produced no output.
</details>
<details>
<summary>Aggregate precheck helper limitation</summary>
```text
/usr/bin/python3: can't open file '/tmp/anyps5-pr13457-precheck/head-d1499225373f-dc2c2885/tools/pr_abi_audit.py': [Errno 2] No such file or directory
/usr/bin/python3: can't open file '/tmp/anyps5-pr13457-precheck/head-d1499225373f-dc2c2885/tools/pr_test_quality.py': [Errno 2] No such file or directory
Negative control
Not run locally: exercising the new C++ assertions requires building the exact PR head. The author reports a negative control in the PR description; I did not independently reproduce it.
Findings
Blocking
The new Iterator.hpp overlaps with #11136, which adds a different declaration of the same public UCharIterator type. The headers are not drop-in interchangeable: #11136 types the move origin as int32_t and omits UCharIteratorOrigin, while this PR uses an enum type; combining the PRs requires reconciling the callback assignments and callers. #13410 also changes the same two export entries that this PR replaces. The repository asks authors to agree on order for overlapping files/exports and record the required Depends on relationship. Please coordinate with those authors, settle the shared header/export order, and update the checklist accordingly.
Non-blocking
The implementation follows the cited ICU algorithm, but no console comparison is available; that limitation is recorded in the debt file.
Sources
- ICU 78.1
u_strCompareIter, pinned to commit 049e0d6a4206 - #11136
Iterator.hppat head 53768b8910a0d540408c4ca9e362bb27599b5fae - #13457
Iterator.hppat reviewed head - #13410 export changes at head c538dc458b454d2037d00e4da8566c319b326627
- Contribution rules at main commit 9304896d89f070f65b8f38bd3a8024d5c19f8fe1
Not verified
I did not build or run the C++ tests locally. Hosted checks were queued at the final refresh. I did not independently verify the PR author's reported Linux test, negative controls, or console behavior.
To get this to a good state
Please coordinate the shared-header and export landing order with #11136 and #13410, then update Depends on to reflect the agreement. I will rerun the same relevant checks on the new head.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
Progress report (7d9a2fc → f01d777)📈 System libraries: 26.71% (+0.0075%, +2 implemented, -2 removed functions) ✅ 2 implemented
🗑️ 2 removed
Changed libraries (implemented exports)
|
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the open PRs #13485, #13479, #13470, #13462, #13459, #13458, #13456, all merged cleanly, so the run is a joint one. AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
…kopovar#13457 Restore the two placeholders that boykopovar#13457 implements, so the two pull requests no longer change the same exports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
u_strCompareIter compares two UCharIterator texts in code unit or code point order as ICU 78.1's ustring.cpp does, driving the guest iterators through their move, next, current and previous callbacks in ICU's order. u_getUnicodeVersion reports Unicode 17.0, the data version of ICU 78.1. A null callback that ICU would call throws std::invalid_argument, and the final difference wraps instead of overflowing. The debt entry records that neither export was compared with a console.
d149922 to
f01d777
Compare
|
Rebased onto current main: #13410 renamed the placeholders next to the two removed stubs, so the removal is re-applied on the renamed file (only Export.cpp conflicted). Checked the new exports against ICU 78.1 as before; the 7 guest_icu_* tests pass on the rebased head; merged. |
Source of the behaviour
Not measured on a device. Both exports follow ICU 78.1 (release-78.1, commit 049e0d6), the ICU version the tree already follows for libSceIcu. The code is written from ICU's contract, not copied.
u_strCompareIter: ustring.cpp L828-L882; the iterator layout and callback order from uiter.h L341-L475, withUITER_STARTat L52.u_getUnicodeVersion: uchar.cpp L484-L489 copiesdataVersion{0x11, 0, 0, 0}, which matchesU_UNICODE_VERSION "17.0".u_strCompareIter(iter1, iter2, codePointOrder)move(iter, 0, UITER_START)on iter1 then iter2, thennexton each in turn until the units differ; both-1(end) gives 0codePointOrdernonzerocurrent, a trail callsprevioustwice), iter1 firstu_getUnicodeVersion(version)Where ICU's behaviour is undefined, the export does not invent a result: a null callback that ICU would call throws
std::invalid_argument(ICU dereferences it), and the final difference wraps as two's-complement arithmetic instead of overflowing.codePointOrderis read as one byte, nonzero meaning true.In-tree lines that change: the two throwing placeholders in Export.cpp (zGuhKwXM9I0, TNZrG2iXikw), the end of Version.cpp, libSceIcu.hpp and the debt file.
What
u_strCompareIter(TNZrG2iXikw) andu_getUnicodeVersion(zGuhKwXM9I0) no longer throw; they behave as the table above shows.u_strCompareIterdrives caller-ownedUCharIterators only through their function pointers, called with the guest ABI, in ICU's order, so a caller's own iterator sees the same calls. The 112-byteUCharIteratorlayout and theUCharIteratorOriginconstants go in a newIterator.hpp, with one function pointer type per callback as uiter.h declares them.Relation to open pull requests:
core/libs/prx/libSceIcu/Iterator.hppdeclaring the same public ICU struct, and addsu_getVersionnext to the same lines of Version.cpp and Version.hpp, as feat(libSceIcu): implement u_getVersion and u_errorName #11115 does. This branch'sIterator.hppwas written separately from uiter.h; the file name is the same on purpose, so the two conflict as files and can never both be built with two definitions ofUCharIterator, and whichever merges second keeps one header. feat(libSceIcu): implement the string and UTF-8 character iterators #11136'suiter_setString/uiter_setUTF8iterators can be compared with this export once both are in.u_strCompareIter, and feat(libSceIcu): implement u_getVersion and u_errorName #11115 addsu_getVersion(notu_getUnicodeVersion) next to the same lines of Version.cpp and Version.hpp.A new debt entry records that neither export was compared with a console and the cases above where ICU is undefined.
Tested
mainwith this branch applied: the newguest_icu_compare_iterpasses, and the other sixguest_icu_*tests still pass. The test drives the export through a guest-ABI iterator that records every callback, and checks: null and identical iterators (no calls), that iterators starting mid-text are reset, equal texts, empty texts, prefixes, code unit order, code point order with a supplementary character on either side, lone leads and trails, pairs that differ in the trail, units above 0xFFFF, acodePointOrderof 0x80, the exact callback sequence of each case, a nullmove,next,currentorprevious(throws only when ICU would call it), and wrapping atINT32_MIN/INT32_MAX.u_getUnicodeVersionwrites 17, 0, 0, 0 and nothing past four bytes, and accepts null.NotImplemented_nid_no_patch, the test fails withu_strCompareIter_nid_postfix not implemented; with onlyu_getUnicodeVersionreverted it fails withu_getUnicodeVersion_nid_postfix not implemented.python3 tools/check_conventions.py --base origin/mainpasses.tools/progress.pyagainstmain: +2 implemented functions (u_getUnicodeVersion,u_strCompareIter); libSceIcu 25/124 to 27/124.tools/pr_overlap.py: reports feat(libSceIcu): implement the string and UTF-8 character iterators #11136 (same new fileIterator.hpp) and feat(libSceIcu): name 74 unknown exports from their NIDs #13410 (renames the same placeholders), both explained above; the other ICU pull requests it could not compare were checked by hand and implement neither export.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)