Repository navigation
Conversation
fscanf, vfscanf, scanf, vscanf and the _s stream forms threw on %ls, %lc, %l[, %S and %C on Linux. Route those formats through the guest scanner sscanf already uses, reading the stream through StreamSource. The wide field reader now works on any source through Peek and Skip instead of a contiguous buffer: it decides whether to take a character from its lead byte, then consumes the whole UTF-8 sequence, so a rejected character stays in the stream without multi-byte pushback. AI-assisted: yes
Deciding from the lead byte only holds while scanset members are ASCII. Each source now decodes its next character with PeekWide without consuming it, and the field reader skips its bytes only once it takes the character. StreamSource peeks by reading the UTF-8 sequence and pushing it back with ungetc, as the fwscanf stream reader in ScanningWide.cpp already does. Keep TSource::Contiguous. AI-assisted: yes
Progress report (f2d874c → 0193ac7)📈 System libraries: 26.26% (+0.01%, +3 implemented functions) ✅ 3 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the scanner changes and the reported build/test results. git diff --check and the current-base conventions check pass. The author reports a full Ubuntu build, the focused scanner tests, and a main-vs-change control for the wider CTest failures; hosted product build jobs are currently skipped.
There is one repository-policy blocker before this can be approved. Both overlapping PRs #9087 and #9340 are still open, and I independently confirmed that merging this head with either branch produces a content conflict in core/libs/prx/libc/src/Formatting.cpp (#9340 also conflicts in docs/dev/debt/libc.md). CONTRIBUTING requires the authors to agree which overlapping PR goes first and declare that order with Depends on. The PR description identifies the overlaps and says the second merge will need a textual rebase, but its checklist still says Depends on: none, and I found no coordination with those authors in the PR discussion.
Please coordinate the merge order with the authors of #9087 and #9340, update the dependency line accordingly, and rebase when appropriate. This is a merge-order request; I did not find a separate correctness blocker in the reviewed code.
Thanks for the review and for checking the conflicts yourself. I've asked the authors of #9087 and #9340 to go first (#9087 comment, #9340 comment) and changed the checklist to |
|
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.
|
|
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 #11644, #11639, #11636, #11635, #11630, #11353, #11347, #11221, #10925, #11706, #11693, #11686, #11682, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
@cocoa55 measurements that bear on this PR's decoding: titles' own What they do. Each byte goes to
Rules that fit every row.
So the UTF-8 decoding on Proposal. I'd like to make the wide branch follow these rules on both platforms. Your PR already makes the branch source-generic, so the natural order is yours first, with mine stacked on it (Depends on: #11724) changing only the per-character step. #11726 should be unaffected beyond a rebase. If you'd rather fold the rules into this PR yourself, say so and I'll stay out of it. Fields cut by Probes and full outputs: AI-assisted: yes (Claude Code). |
|
@cocoa55 following up on my note above: I've opened the stacked follow-up as #12468 (Depends on: #11724), so it's reviewable alongside yours. Your two commits come first there, unchanged. If you'd still prefer to fold the byte-by-byte rule into this PR, say so and I'll close #12468. AI-assisted: yes (Claude Code). |
Thanks a lot for measuring this on the titles’ libc.prx, that’s much better evidence than the UTF-8 assumption both main and this PR carry. I’d prefer to keep them separate: #11724 stays “wide conversions work on streams” with today’s decoding, and #12468 switches both strings and streams to the byte-by-byte rule on top. Happy to rebase or adjust PeekWide if it makes #12468 simpler. |
boykopovar#12530 reads source.Remaining() in the contiguous branch of ScanSource, so dropping the accessor here would break the build whichever of the two lands second, without a textual conflict. AI-assisted: yes
…-fields # Conflicts: # docs/dev/debt/libc.md
|
Linux run of head
The control aborts with #11726 contains this PR's commits; my run of #11726 is there. AI-assisted: yes (Claude Code) |
|
@cocoa55 I opened #13696 for the same stream forms before I saw this PR. I'm closing it in favour of this one. Two things from it that may be useful here:
AI-assisted: yes (Claude Code) |
…-fields # Conflicts: # docs/dev/debt/libc.md
✅ AMD GPU test: passedPR head
Unstable on main (6, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 711 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
On Linux,
fscanf,vfscanf,scanfandvscanfthrewNotImplementedon the 16-bit wide conversions (%ls,%lc,%l[,%S,%C), and so did the_sstream forms.sscanfalready reads them with the guest scanner (ScanSource), which also has aStreamSource, but its wide branch only ran for contiguous strings (if constexpr (TSource::Contiguous)). The UTF-8 decoding is the existing sscanf behaviour; #12468 replaces it with the byte-by-byte rule measured on titles’ libc.prx.ScanSourceno longer walks aconst char*. Each source getsPeekWide, which decodes the next character without consuming it, and the branch callsSkipfor its bytes only once the field takes the character.StringSource::PeekWideis the oldReadWideScanCharactercall.StreamSource::PeekWidereads the UTF-8 sequence withfgetcand pushes it back withungetc, as thefwscanfstream reader inScanningWide.cppalready does. A rejected character therefore stays in the stream, and the accept/reject logic is the same code for strings and streams.fscanf/vfscanfuse the scanner when the format has a wide conversion (same check assscanf,ScansWideInProcess) and the hostvfscanfotherwise.ScanGuestStream(fscanf_sand friends) no longer rejects wide conversions and keeps the positional-argument check.RejectGuestWideScanis removed.ScanWideFieldgetsWideScanCharacterLength, whichDecodeUtf8now uses too.docs/dev/debt/libc.md: the scanf family and_sstream entries no longer listfscanfas throwing.Overlap: #9087 and #9340 change the same wide branch of
ScanSource(secure-size violations, and surrogate pairs / non-ASCII scansets). Neither reads wide fields from a stream, so they don't duplicate this PR. Agreed order (asked on both PRs): #9087, then #9340, then this PR, which I'll rebase after each one merges. StringSource::Remaining is kept because #12530 uses it. #11726 and #12468 build on this PR.Tested
Ubuntu 24.04, GCC 13.3, full build. New
CheckNarrowFormatWideFieldsincore/libs/tests/GuestWideStreamScan.cpp(Linux only, like thesscanfwide tests):fscanf%ls/%lcon UTF-8,vfscanf%Swith%ln, wide and narrow fields in one call, a scanset stopping beforeéand theéread by the next call,%3lcwith the next character left in the stream, matching failure and EOF,fscanf_s/vfscanf_ssizes,scanf/vscanfon the guest stdin. It fails onmainwithfscanf 16-bit wide conversion on Linux not implemented.guest_formatting,guest_scan_count,guest_wide_stream_scanandfile_streamspass. Fullctest: 2476 of 2491 pass, and the 15 failures (agc_*submit/metadata,guest_net_*,guest_tcp,guest_socket_*) fail the same way onmainin the same container (no GPU, sandboxed network). No title tested.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)