Repository navigation
feat(libSceIcu): implement 458 more data-free ICU 68 exports - #14340
Open
PrivacyOnion wants to merge 4 commits into
Open
PrivacyOnion wants to merge 4 commits into
PrivacyOnion wants to merge 4 commits into
Conversation
MarcZX10
reviewed
Nov 1, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Tests run and reviewed — evidence requested
The exact head passed the no-build conventions and public-header checks, but the promised differential/layout harnesses and logs for these 458 additions are not attached; I am not approving this change yet.
What I ran
Reviewed head a922117012130a5b4e0e67dd3abfdbc2dd39a2b8 against current main at 26fa1212b82f76dd49cf12d65e445d6ff3e0844c; dependency and individual merge previews are clean. The incremental diff is one commit after #14144 at 919905beb491a418e946e79f4298cc988fb4d15b. Environment: Linux x86-64, Python 3.11.2, GCC 12.2.0 (c++). I did not build locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14340 --output-dir /tmp/anyps5-pr-precheck-14340
git diff --check 26fa1212b82f76dd49cf12d65e445d6ff3e0844c...a922117012130a5b4e0e67dd3abfdbc2dd39a2b8
python3 core/libs/tests/test_public_headers.py --audit-only
python3 tools/pr_stack_check.py 14340 --repo boykopovar/AnyPS5
gh pr checks 14340 -R boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff whitespace | Failed on one trailing-whitespace line in UnicodeSet.cpp and an extra blank line at the end of CMakeLists.txt |
| Conventions and PR template | Passed locally |
| Public-header audit | Passed: 394 libraries, 26,624 exports |
| Dependency/merge preview | Clean; #14016 → #14144 → #14340 |
| Hosted Emscripten portability | Passed |
| Other hosted checks | Windows, conventions, intake, and labels pending; Linux and specialized build jobs skipped |
Unedited local check output
core/libs/prx/libSceIcu/CMakeLists.txt:873: new blank line at EOF.
core/libs/prx/libSceIcu/UnicodeSet.cpp:351: trailing whitespace.
+
All conventions checks passed.
394 libraries, 26624 exports: unique declarations and umbrella coverage verified
Negative control
| Assertion | Change to code under test | Result |
|---|---|---|
| Differential/layout assertions for the newly added exports | No local build or harness run | Not independently exercised; results in the PR description are author-reported |
Findings
Blocking
None established from the source and no-build checks performed so far.
Non-blocking
- Please remove the two whitespace findings so
git diff --checkpasses. - The PR says the 24 new differential harnesses, layout checks, pins, and per-harness logs will be attached in a comment, as they were for #14144. I do not see that attachment on this PR yet. Because this change adds hundreds of ABI-sensitive implementations and the automatic guest build/test jobs are not running, those materials are needed to assess the reported coverage before approval.
Sources
- Changed UnicodeSet implementation at the reviewed head
- Test registration at the reviewed head
- ICU 68.2 source revision named by the PR
Not verified
I did not run a guest build, CTest, the differential harnesses, or the layout harnesses. The Windows tests and guest ABI object compilation in the PR description are author-reported. No game or console behavior was tested.
To get this to a good state
Attach the new harness sources, build pins/commands, and per-harness logs; remove the whitespace findings. I will re-run the same no-build checks on the refreshed head and reassess the test evidence.
Progress report (d1ae628 → 40f5f73)📈 System libraries: 29.02% (+2.55%, +941 implemented, +761 declared, -1695 removed functions) ✅ 941 implemented
⚪ 18 behaviourally empty (constant return or throw)
🆕 761 declared as stubs
🗑️ 1695 removed
Changed libraries (implemented exports)
|
Collaborator
|
@PrivacyOnion Five points from the incremental change over
|
PrivacyOnion
force-pushed
the
feat/icu-impl3
branch
from
November 1, 2026 10:45
a922117 to
f6303c9
Compare
Author
|
Updated in f6303c9.
@MarcZX10 @oneandonlydean The trailing whitespace in |
|
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.
|
Author
|
Harnesses, run script, pins and per-harness logs for f6303c9 (the 30 from #14144 plus 24 for this PR, all run against the merged tree): https://gist.github.com/PrivacyOnion/fd4bdd72c96d48e0b127722842ac656c (README.md lists the ICU commit, the build commands and the expected CharStringDiff separator mismatch). |
MarcZX10
reviewed
Nov 1, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — new head, no blocking findings
The author asked for a re-review at f6303c9c46; the whitespace and harness requests from the earlier review are addressed, and the open points are an ICU licence/notice question for the maintainers and the dependency on #14144.
What I ran
Reviewed head f6303c9c4669d5a2fa8efea8dc291ce76acf788c. Incremental diff base 919905beb491a418e946e79f4298cc988fb4d15b (the #14144 head that this PR stacks on). PR base e6f57307fa7f47165d7fc1bee945abdb2f6816e7, used for the licence-precedent count. Environment: Linux x86-64, Python 3.11.2. No build, no compile, no CTest run on this machine.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14340 --output-dir /tmp/anyps5-pr-precheck-14340-f6303c9
git diff --check 919905beb491a418e946e79f4298cc988fb4d15b f6303c9c4669d5a2fa8efea8dc291ce76acf788c
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base 919905beb491a418e946e79f4298cc988fb4d15b --head f6303c9c4669d5a2fa8efea8dc291ce76acf788c --json
python3 tools/pr_test_quality.py --root <git archive of core/libs/prx/libSceIcu and core/libs/tests at f6303c9> --json <the 24 test files added in f6303c9>
git grep -l 'unicode.org/copyright' e6f57307fa7f47165d7fc1bee945abdb2f6816e7 -- core
git grep -l 'unicode.org/copyright' f6303c9c4669d5a2fa8efea8dc291ce76acf788c -- core
git grep -l -i 'permission is hereby granted' f6303c9c4669d5a2fa8efea8dc291ce76acf788c -- .The precheck's ABI and test-quality helpers were missing from the isolated head tree, so I ran tools/pr_abi_audit.py and tools/pr_test_quality.py by hand from /marc/AnyPS5/tools against the exact head.
Results
| Check | Result |
|---|---|
Diff whitespace, 919905b..f6303c9 and e6f5730..f6303c9 (precheck) |
Passed. The trailing-whitespace and end-of-file findings from the earlier head no longer appear. |
| Conventions and PR template (precheck) | Passed |
Live dependency and mergeability (precheck pr_stack_check.py) |
Passed; see Findings on #14144 and #14123 |
Hosted windows job (run 41298866382, job 124827010833) |
Passed. The job merges the PR into main before it builds, so this covers that merge result, not the bare head. The log has the CTest summary 100% tests passed, 0 tests failed out of 2631 (log line 15642), including guest_icu_service_key ... Passed. |
Hosted library-portability, conventions, labels, report, eligibility |
Passed. linux, relinker, relinker-asan, register-context and write-watch were skipped. |
| Test registration | Passed by manual check. All 53 files under core/libs/prx/libSceIcu/tests/ are referenced by name in CMakeLists.txt. The precheck's helper reported 24 possible-unregistered-cpp-test findings; they come from the helper not resolving ${CMAKE_CURRENT_SOURCE_DIR}/tests/, so they are false positives. |
| Incremental ABI audit (manual) | passed_with_warnings: 1906 symbols, 13 lexical public-declaration-not-found entries and no export-removed entries for this commit. I traced one entry, Notify, which is a callback in tests/ServiceNotifier.cpp; I did not trace the other twelve. |
Test-file assert scan |
No assert( in the 24 new test files. |
| Licence precedent | unicode.org/copyright header: 0 files at base e6f5730, 124 files at head. Permission-notice text: 0 matches in the head tree. |
Points raised in oneandonlydean's 10:26 comment, checked in source at this head:
| Point | Check at f6303c9 |
|---|---|
| 1. Shared locale cache | LocaleObject.cpp declares std::once_flag CacheOnce; and publishes the cache inside std::call_once (lines 97 to 100). |
| 2. Release-safe checks | tests/ServiceKey.cpp uses a Check() that calls std::abort(), and no assert( remains in the 24 new test files. I read only ServiceKey.cpp in full. |
| 4. Copyright blocks | LocaleObject.cpp keeps IBM's lines after the Unicode notice; 46 libSceIcu files contain IBM copyright text. I did not check every file. |
| 5. ICUService debt entry | docs/dev/debt/libSceIcu.md: the ICUService entry no longer names a host mutex; the host mutex is described only under ICUNotifier. |
Author-supplied evidence, not rerun here: the differential and layout harnesses are in a gist (55 files, README pins ICU release-68-2 at 84e1f26). 53 of its 54 logs record DIFF EXIT CODE: 0. The exception is CharStringDiff (exit 1), which the author attributes to the Windows reference build using \ as the separator, where the PS5 build uses /.
Findings
Blocking
None established from the source, the hosted results and the author's evidence.
Non-blocking
- ICU notice and licence treatment (maintainer decision). The new files reproduce ICU's header and the copyright lines of the ICU files they follow, which the PR body describes as ports written "from the ICU 68.2 sources function by function". ICU's licence at the pinned commit says the copyright and permission notice must "appear with all copies of the Data Files or Software, or ... appear in associated Documentation" (icu4c/LICENSE). The head tree does not contain the permission-notice text, and the new headers link to the Unicode terms instead. Whether the notice text must be carried, and how ports should be attributed, is for the maintainers to decide. I am not giving a legal opinion, and I am not requesting changes on this point.
- Checklist item. The PR body ticks "New third-party code is a submodule built from source". CONTRIBUTING.md line 17 says third-party code is added as a submodule under
3rdparty/, and this PR adds no submodule. Either the rule does not cover hand ports, or it needs a maintainer decision. The checkbox should match whichever it is. - Where the differential evidence lives. The harnesses are in a gist, not in the repository. Their
HostAbi.hppdefinesAPS5_VABIas empty and they were built with MinGW on Windows, so they test the host ABI, not the console's. The repository-side evidence is the hosted Windows CTest run above. If the maintainers want the differential evidence to be reproducible, it needs a stable location, either in the repository or linked from the debt file. - Dependency. The PR stacks on #14144 (head
919905beb4, open,mergeableUNKNOWN). The PR body says it lands after #14123 (open, mergeable). No change is requested now. Re-check after #14144 merges or when the head changes.
Sources
- Head of the PR,
LocaleObject.cppheader andCacheOnceuse: https://github.com/boykopovar/AnyPS5/blob/f6303c9c4669d5a2fa8efea8dc291ce76acf788c/core/libs/prx/libSceIcu/LocaleObject.cpp - Service test with
Check(): https://github.com/boykopovar/AnyPS5/blob/f6303c9c4669d5a2fa8efea8dc291ce76acf788c/core/libs/prx/libSceIcu/tests/ServiceKey.cpp - Debt entries: https://github.com/boykopovar/AnyPS5/blob/f6303c9c4669d5a2fa8efea8dc291ce76acf788c/docs/dev/debt/libSceIcu.md
- Contribution rule:
Line 17 in f6303c9
- ICU licence at the pinned reference commit: https://github.com/unicode-org/icu/blob/84e1f26ea77152936e70d53178a816dbfbf69989/icu4c/LICENSE
- Hosted Windows job: https://github.com/boykopovar/AnyPS5/actions/runs/41298866382/job/124827010833
- Author's harness gist (not rerun here): https://gist.github.com/PrivacyOnion/fd4bdd72c96d48e0b127722842ac656c
Not verified
- No build, no CTest, no harness rerun on this machine. The gist logs, the ICU 68.2 comparisons and the author's ServiceKey negative control (
Checkaborts with exit code 3 when the expected descriptor is changed topre/abX, under-O2 -DNDEBUG) are author-reported. - The Linux guest build and CTest were skipped by hosted CI for this PR, and no console or PS5 behavior was tested.
- The full-PR ABI audit against
e6f57307also lists 1695 removed legacy exports. They come from the renames inc516806, which belong to #14016 and #14144; I did not re-verify their NID equality here. - The 13 incremental lexical findings were not traced one by one beyond
Notify. - The public-header audit was not rerun at
f6303c9. - Point 3 from oneandonlydean's comment (the description matching the submitted subset) was checked only against the PR text and the debt file, not export by export.
To get this to a good state
- Maintainers: decide points 1 and 2. If the notice text must be carried, the author adds it in the place the maintainers choose and updates the checklist line to match.
- No code change is requested at this head. Re-check after #14144 merges or on any new head.
Author
|
@MarcZX10 thanks for the re-check. On the two maintainer points: the checklist line now says that no submodule is added, and that the ICU-derived files are rewrites carrying ICU's notice plus the original copyright lines. If the maintainers want the full ICU permission text, or a pointer from the debt file to the harness gist, I'll add it wherever they prefer. I will rebase this PR once #14144 (and #14123 before it) land. |
Open
12 tasks done
THEMANnt
approved these changes
Nov 1, 2026
THEMANnt
left a comment
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed f6303c9c4669d5a2fa8efea8dc291ce76acf788c, focusing on its increment over #14144. No actionable issue found in the reviewed implementation paths.
Windows/MSYS2 GCC 16.2 Release: built the shared library and passed all 57 supplied ICU suites in an isolated DLL-linked harness. Independent seeded checks pass 5,000 UnicodeString mutation/copy-on-write rounds, 10,000 integer-vector rounds and 10,000 UnicodeSet range add/remove/complement rounds against simple reference models. Hosted Windows reports 2,631/2,631 selected tests passing. Whitespace and conventions pass.
Local support substitutes minimal libc functions. This is not exhaustive differential validation of all 458 exports, allocation failures or console behavior. Depends on #14144 and its merge ordering; maintainer integration/notice decisions remain with the maintainers.
added 3 commits
November 2, 2026 17:47
1691 legacy and 4 dispatch placeholders are renamed to the ICU 68.2 symbol whose NID hash equals the 3.20 NID, so they export the same NIDs through _nid_postfix names instead of explicit APS5_EXPORT entries. The dispatch NIDs are the BreakIterator::create*Instance factories. All stubs still throw through NotImplemented_nid_no_patch.
Hash tables, lists, string enumerations, tracing, platform helpers, cleanup, termination helpers, code point tries, UTrie2, UText core, UnicodeString, CharString, StringPiece, Appendable, ByteSink, MaybeStackArray, PatternProps, ICU_Utility, UVector, UVector32, UStack, Edits, BytesTrie, UCharsTrie and character iterators follow ICU 68.2 with the console's LP64 layouts and guest virtual tables. Exports that need ICU data, converters or memory released through uprv_free keep throwing NotImplemented_nid_no_patch.
The ICU service framework, UnicodeSet with its iterator, filter and pattern parser, Locale, LocaleBuilder and the uloc ID helpers, and the remaining character iterators, string enumerations, vector objects, ErrorCode, ByteSink utilities and UTrie2 enumeration follow ICU 68.2 with the console's LP64 layouts and guest virtual tables. Object.cpp exposes its type-info tables so derived classes build their own RTTI. Exports that need ICU data, converters or case tables keep throwing NotImplemented_nid_no_patch.
PrivacyOnion
force-pushed
the
feat/icu-impl3
branch
from
November 2, 2026 16:57
f6303c9 to
971ae50
Compare
12 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements 458 more libSceIcu exports that need no ICU data, as reimplementations of ICU 68.2, the version of the 3.20 module (its C++ names are in
icu_68). Builds on #14144 (which includes #14016); this PR adds one commit on top of it and only replaces stubs that #14016 named.ICUService,ICUServiceKey,ICUServiceFactory,SimpleFactory,LocaleKey,LocaleKeyFactory,SimpleLocaleKeyFactory,ICUNotifier,EventListener,ServiceListener,StringPair,LocaleUtilityUnicodeSet(construction from ranges and copies, set algebra on code points, ranges and strings,cloneAsThawed, serialization,toPattern, comparison, hashing),UnicodeSetIterator,UnicodeFilter,UnicodeFunctor,UnicodeMatcher,SymbolTable,UStringSet,ParsePositionLocale(IDs, getters, keywords, copy/assignment, hashing),LocaleBuilder,LocaleBased,uloc_*ID helpersForwardCharacterIterator,CharacterIterator,UCharCharacterIteratorandStringCharacterIteratorobjectsStringEnumeration,UStringEnumeration,uenum_openFromStringEnumerationUVector,UStack,UVector32constructors and destructorsErrorCode,CharStringByteSink,ByteSinkUtil,ICU_Utilityuhash_hashUnicodeString/uhash_compareUnicodeString,utrie2_enum/utrie2_enumForLeadSurrogate,UnicodeString::toUTF8(ByteSink&)and four remainingStringPiece/CharString/MaybeStackArraymembersLayouts follow ICU 68.2 for LP64 and are pinned with
static_assert.UnicodeSetreproduces ICU's multiple inheritance fromUnicodeFilter(UnicodeFunctor+UnicodeMatcher): both vtables and the_ZThn8_thunks are in place. Classes with virtual functions get staticAPS5_VABIvtables and type-info objects in ICU 68.2's slot order.Object.cppnow exposes its__class_type_info,__si_class_type_infoand__vmi_class_type_infotables so derived classes build their own RTTI; no_ZTV/_ZTIexports are added. The new files were written from the ICU 68.2 sources function by function, so each carries ICU's notice (© 2016 and later: Unicode, Inc. and others, http://www.unicode.org/copyright.html) followed by the copyright lines of the ICU files it follows (for example IBM's forlocid.cppandserv.cpp).Still throwing
NotImplemented_nid_no_patchand recorded indocs/dev/debt/libSceIcu.md:\p{…}/[:…:],applyIntPropertyValue,applyPropertyAlias, case closure), locale display names, likely subtags, resource-bundle factories and available-locale lists;UnicodeStringmembers;UnicodeSet::freeze, thespan*calls,applyPattern,applyPatternIgnoreSpace, the pattern constructor andcontainsAll/containsNonewith a string, which needBMPSet,UnicodeSetStringSpanand theRuleCharacterIteratorpattern parser;umutablecptrie_buildImmutable, theudataand format swappers, and theutextproviders for UTF-8,UnicodeString,ReplaceableandCharacterIterator, left for a follow-up.The exports owned by #12320, #13578 and #14123 are left untouched.
Tested
Windows 11, MinGW GCC 13.1. No title run.
release-68-2at84e1f26ea77152936e70d53178a816dbfbf69989, all 196common/*.cppfiles plusstubdata). Each implemented export is called with the same inputs as ICU's: edge cases, fixed-seed random inputs, and random operation sequences of a few hundred calls for classes. Every return value, output buffer,UErrorCodeand object field is compared. All pass except the knownCharStringDiffseparator case from feat(libSceIcu): implement 481 data-free ICU 68 exports #14144 (the host reference is a Windows build).UnicodeSet(primary andUnicodeMatchervtables),UnicodeSetIterator,ParsePosition,Locale,LocaleBuilder, the service classes, the character iterators, the string enumerations, the vector objects andErrorCode: 0 mismatches. The harnesses and their logs will be attached in a comment, as for feat(libSceIcu): implement 481 data-free ICU 68 exports #14144.APS5_VABIand the CI flags. The guest build and CTest are left to CI.python3 tools/check_conventions.py --base upstream/mainandpython3 core/libs/tests/test_public_headers.py --audit-onlypass.tools/pr_overlap.pyreports no shared exports. The only shared lines are the libSceIcuCMakeLists.txt,libSceIcu.hppinclude list and debt file.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)