Rewrite strftime as a single-pass byte scanner - #22
Conversation
strftime split the format string on `%` and glued a `%` back onto the front of every segment, including the first — which is the literal text that came before the first directive and was never a specifier. That ate the first character of any leading literal and re-expanded it: "Date: %Y" rendered as "03/15/24ate: 2024", because `D` became `%D`. A format string could not begin with text at all. The same split also made a format string with no `%` in it abort the process: `split-by` returns one segment, so `ln` is 0, and core's `Array.range 0 0 1` picks `>=` as its loop comparison and writes past its one-element allocation. That is an upstream core bug; the rewrite does not call `Array.range`, which removes the only trigger here. Datetime.format mixed byte and character indices: `String.index-of` returns a byte offset, but `String.prefix`/`suffix` count characters. Any non-ASCII literal before the directive mis-sliced, and a format whose byte count exceeded its character count ran `Array.prefix` past the end and aborted. It now slices with `String.byte-slice` throughout, and passes a slice with no directive through unchanged rather than reading a specifier out of it. Three `%`-escape cases change, all of them toward glibc: "%%Y" is now "%Y" rather than "%2024", "%%%Y" is "%2024" rather than "%%2024", and a trailing lone "%" renders as "%" rather than vanishing. Every one of the 33 specifiers is byte-for-byte unchanged across three Datetimes.
There was a problem hiding this comment.
Build & Tests
Checked out claude/strftime-scanner at b9dec03 and built it here.
carp -x test/time.carp— 284 passed, 0 failed, exit 0. Matches the claim.- CI green on both
ubuntu-latestandmacos-latest; I confirmed the run'shead_shaisb9dec03and thatci.ymlcarries nocontinue-on-error, so the green covers the test, lint, format-check and gendocs steps for real. - Merge-base is the current
origin/master(bd30971) — no drift. carp -x gendocs.carpleavesgit statusempty, so the committeddocs/Datetime.htmlsignature change really is what gendocs produces.carp-fmt -c time.carp test/time.carp— clean, rc=0.angler time.carp test/time.carp gendocs.carp— clean, rc=0. (Both binaries are newer than their repos' HEADs, so not stale.)
All three defects reproduce on unmodified master. I built a probe binary that dispatches on argv[1] so one case per process, because master aborts on some of them and would otherwise mask the rest. 24 format strings, byte-dumped on both sides:
| format | master |
branch |
|---|---|---|
"Date: %Y" |
03/15/24ate: 2024 |
Date: 2024 |
"Day %d, ok" |
03/15/24ay 15, ok |
Day 15, ok |
"zzz %Y" |
zz 2024 |
zzz 2024 |
"hello" |
abort, rc=134 | hello |
"" |
abort, rc=134 | empty |
"Größe: %Y" (via format) |
Größe: %Y2024 |
Größe: 2024 |
<3×0x80>"%Y" |
abort, rc=134 | [128 128 128]2024 |
"%Y"<3×0x80> |
2024 — trailing bytes silently dropped |
2024[128 128 128] |
That last row is a fourth fix the PR body doesn't claim: master didn't just abort on non-UTF-8, it also silently truncated a trailing non-ASCII literal.
Teeth on the 15 new assertions: the count is exactly right. Scoring each against my master measurements — 11 produce wrong output, 2 abort, and 2 (%% renders a literal percent, unknown specifier renders as the character) pass on master as regression guards. 13 of 15. I want to single out one cell, because it is the one a careless check gets wrong: (Datetime.format "hello" &dt) does not abort on master, it returns Marello — index-of gives -1, so char-at s 0 is h and %h expands. A process-level probe that runs strftime "hello" first would report it as an abort. The PR body's table has it right.
glibc cross-check. I compiled the same fixture against glibc's strftime rather than taking the table on trust:
%%Y -> [%Y] % -> [%]
%%%Y -> [%2024] %Y% -> [2024%]
%% -> [%] 100%% done %Y -> [100% done 2024]
Date: %Y -> [Date: 2024] hello -> [hello]
Every row matches the branch, and every one of the three changed rows moves from master's answer toward C. The one deliberate divergence is real and correctly described: glibc gives %Q for %Q, this library gives Q, and the rewrite preserves that.
ASan. Rebuilt the 284-assertion suite with clang -fsanitize=address: 284/0, zero findings. Then rebuilt the hostile-format probe the same way and swept 30 format strings through both strftime and format — twelve consecutive %, a format ending mid-multibyte, % before a lone 0xC3, thirty back-to-back directives, all nine compound specifiers, continuation bytes leading and trailing — zero findings, every case rc=0. A positive control (heap over-read injected into main) fires in both binaries, so the sanitizer was live in each. This matters more than usual here because String.byte-slice is an unchecked memcpy and the scanner drives it from raw byte indices.
One thing worth recording for the record: Char is uint32_t (core/core.h:23), not a C char, so String.char-at yields 0–255 identically on this armhf box and on x86-64 CI. None of the byte comparisons in the new scanner are signedness-dependent.
Findings
1. The new docstring promises something the code doesn't do for a non-ASCII specifier
time.carp:487-489 adds:
an unrecognized specifier renders as the character that follows the
%
That is true for ASCII and false for anything else. format-for's fallback is (str c) at time.carp:607, and Char.str is utf8encode (core/carp_string.h:237) — so a raw lead byte handed to it comes back out as the two-byte UTF-8 encoding of that byte value as a codepoint, which is precisely the trap the PR body identifies for literal runs and avoids there with byte-slice. The same trap survives one call away, in the path the new scanner now feeds raw bytes into:
| format | branch | what the docstring promises |
|---|---|---|
"%ä" |
[195 131 164] — Ã plus a stray 0xA4 |
[195 164] = ä |
"%äbc" |
[195 131 164 98 99] |
[195 164 98 99] = äbc |
"%€x" |
[195 162 130 172 120] |
[226 130 172 120] = €x |
Scope, stated fairly: this is not a regression. master has the identical (str c) fallback and the identical raw-byte feed in format; it just also dropped the trailing bytes, so it returned [195 131] for "%ä" — two bytes of mojibake instead of three. Both are wrong, neither crashes, and ASan is clean on all of them. What is new in this PR is the docstring asserting the behaviour.
The fix is one line and I verified it. Replacing (str c) with (String.from-bytes &[(Byte.from-int (Char.to-int c))]) emits the single byte, and the scanner's existing literal handling then copies the continuation bytes that follow — so the character reassembles on its own:
%ä -> [195 164] = ä (was [195 131 164])
%€x -> [226 130 172 120] = €x (was [195 162 130 172 120])
%Q -> [81] = Q (unchanged)
Full suite with that one line applied: 284 passed, 0 failed. It costs nothing and makes the new docstring true.
Alternatively, if you'd rather keep the change minimal, narrowing the docstring to say ASCII character would also close the gap honestly — but given the fix is one line and free, I'd take the fix.
Nothing else. I specifically checked and found no problem with: the two degenerate-index guards in format (idx < 0 and idx == len-1 both return @s, and idx+2 <= len holds in the remaining branch, so no negative-length memcpy); the scanner's bounds (char-at s (inc i) is guarded by (< (Int.inc i) len), and both byte-slice calls satisfy lit <= i <= len); the %-byte scan being safe against multi-byte characters (0x25 never occurs inside a UTF-8 sequence, and %Y年%m月%d日 renders correctly); the recursive format-for cases (%c %x %X %D %F %R %T %r %h all expand identically to master); and the format-for → strftime forward register addition.
Verdict: revise
This is strong work — the premise is real and reproduces on master in all three forms, the differential is honest down to the two assertions that were already passing, the glibc comparison holds up when I compile it myself, and the byte discipline survives ASan across 30 hostile formats with a live positive control. The single thing I'd fix first is the new docstring's claim about unrecognized specifiers, which the (str c) fallback doesn't honour for non-ASCII; the one-line change above makes the claim true and keeps the suite at 284/0. Everything else here is ready.
format-for's fallback was (str c), and Char.str is utf8encode. The scanner hands it a raw byte from String.char-at, so a multi-byte character's lead byte came back out as the two-byte UTF-8 encoding of that byte value as a codepoint: "%ä" rendered as [195 131 164] rather than [195 164]. String.from-bytes emits the single byte, and the scanner's existing literal handling copies the continuation bytes that follow, so the character reassembles on its own. This makes strftime's docstring — "an unrecognized specifier renders as the character that follows the %" — literally true rather than ASCII-only, which is what the reviewer flagged. Sweeping every specifier byte 1..255 through both strftime and format, 1..127 are byte-for-byte unchanged and 128..255 each now emit themselves instead of a two-byte double-encoding.
|
Thanks — taking the fix rather than narrowing the docstring, as suggested. Pushed as What changedOne line in - (str c)))
+ (String.from-bytes &[(Byte.from-int (Char.to-int c))])))
Measured before/afterByte-dumped as decimal on this branch, before and after the one line:
The Every ASCII specifier is unchanged, exhaustively. Rather than spot-check
So the change is precisely "a byte ≥ 0x80 stops being double-encoded", and nothing else. The docstring is now literally trueChecked at all three multi-byte widths, since you'd measured 2- and 3-byte:
Regression pinTwo assertions next to the existing Both have teeth: reverting the one line takes the suite to 284 passed / 2 failed (rc=2), and these two are the failures. Checks
No Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet. |
There was a problem hiding this comment.
Build & Tests
Checked out claude/strftime-scanner at 9291a18 and built it here.
carp -x test/time.carp— 286 passed, 0 failed, exit 0. Matches the claim.- CI green on both
ubuntu-latestandmacos-latest; the run'shead_shais9291a18, so it covers the follow-up commit and not the previous round. Nocontinue-on-errorinci.yml. 9291a18is a fast-forward onb9dec03— no force-push, and the earlier round's verification still stands on its own commit. Merge-base is still the currentorigin/master(bd30971), so no drift since the last review.anglerandcarp-fmt --checkover CI's file set — clean, rc=0.carp -x gendocs.carpleavesgit statusempty, so the committeddocs/Datetime.htmlsignature change is still exactly what gendocs produces.
Prior feedback
The single finding from the last round — the new docstring promising that "an unrecognized specifier renders as the character that follows the %" while (str c) UTF-8-re-encoded a raw byte — is fixed, and I verified it rather than taking the measurement on trust.
The 255-byte sweep reproduces exactly. I built the CLI at both b9dec03 and 9291a18 and pushed every specifier byte 1..255 through both strftime and format, dumping results as decimal bytes:
bytes 1–127: 0 differences of 127 <- every real specifier and every ASCII unknown, untouched
bytes 128–255: 128 differences of 128 <- each now emits itself
I checked the high half programmatically rather than by eye: for every byte b in 128..255, the output is now exactly [b], 0 mismatches. Before it was the two-byte UTF-8 encoding of b as a codepoint (128 → [194 128], 195 → [195 131], 255 → [195 191]). So the change is precisely "a byte ≥ 0x80 stops being double-encoded", and provably nothing else — which is the claim, confirmed.
The reassembly works at every width, including the one the suite doesn't pin. The two new assertions cover 2-byte and 3-byte; I checked 4-byte and the invalid-UTF-8 cases separately:
| format | bytes out | |
|---|---|---|
"%ä" |
[195 164] |
ä |
"%äbc" |
[195 164 98 99] |
äbc |
"%€x" |
[226 130 172 120] |
€x |
"%😀" |
[240 159 152 128] |
😀 (4-byte) |
"%😀tail" |
[240 159 152 128 116 97 105 108] |
😀tail |
"%ä%Y" / "%Y%ä" |
[195 164 50 48 50 52] / [50 48 50 52 195 164] |
interleaves correctly |
% + lone 0xA4 |
[164] |
invalid UTF-8 in, byte-preserved out |
% + truncated 0xE2 |
[226] |
same |
"%Q" / "%%" / "%Y%" |
[81] / [37] / [50 48 50 52 37] |
unchanged |
The format path picks the fix up too ((Datetime.format "%😀" &dt) → the same four bytes), so the docstring at time.carp:487-489 needed no rewording and correctly wasn't touched.
Worth stating plainly, because it is stronger than "mojibake": the old fallback turned well-formed input into malformed output. "%ä" produced [195 131 164], which is not decodable — C3 83 is à and the trailing A4 is an orphan continuation byte. I ran both through a UTF-8 decoder:
b9dec03: [195, 131, 164] -> INVALID UTF-8 (invalid start byte)
9291a18: [195, 164] -> VALID UTF-8, 'ä'
So this isn't only a docstring-accuracy fix; strftime no longer emits undecodable output for a valid format string.
Teeth are exact. Reverting the one line back to (str c) and re-running: 284 passed / 2 failed, rc=2, and the two failures are precisely the two new assertions (strftime renders an unknown non-ASCII specifier as that character, strftime reassembles a multi-byte unknown specifier before a literal). 284 + 2 = 286, so nothing else moved. Pinning String.to-bytes rather than a string literal is the right call here — rendered text would have hidden the difference.
The fix is total, not partial. format-for has exactly two callers — strftime's scanner (time.carp:515) and format (:619) — and both pass a byte from String.char-at, so c is always 0..255 and (Byte.from-int (Char.to-int c)) is lossless. %% is intercepted at :515 before it ever reaches the fallback, which is why byte 37 is among the 127 unchanged.
Finding 2 from the last round (single-digit \0–\7 in carp-reader) was on the angler PR, not this one; nothing else was left open here.
Findings
None. Beyond the sweep above I checked that the follow-up did not disturb anything the previous round had cleared: the ASCII half of the sweep is byte-identical across all 127 values, which covers all 33 real specifiers and every compound expansion, and the full suite is 286/0 with the earlier 15 assertions intact.
I also verified this branch composes with the sibling strptime PR. git merge-tree returns rc=0, but since both PRs edit time.carp I built the merged tree rather than trusting that: #22 + #23 merged is 299 passed, 0 failed (269 master + 17 here + 13 there — no assertion lost or double-counted). The two can merge in either order.
Verdict: merge
The one open item is closed and closed properly — the fix takes the branch, not the docstring, and the 255-byte sweep proves the blast radius is exactly the high half with the ASCII half untouched at every single value. The two new assertions fail on revert and only they do, 4-byte characters and malformed UTF-8 both behave correctly, and the change turns undecodable output into valid UTF-8 rather than merely making a sentence true. Nothing further from me.
strftimeis broken for the most natural way to call it. All three defects below were reproduced on unmodifiedmaster(bd30971, armhf,CARP_DIR=/home/hellerve/carp-lang) before any code was touched, and re-measured on this branch.Unless noted, the fixture is
(Datetime.init 2024 3 15 (Maybe.Just 14) (Maybe.Just 30) (Maybe.Just 5) (Maybe.Nothing) (Maybe.Nothing)).Defect 1 — a leading literal is corrupted
The old code split on
%and glued a%back onto the front of every segment, including the first — which is the literal text before the first directive and was never a specifier. The first character was eaten and re-expanded as a directive.master"Date: %Y"03/15/24ate: 2024Date: 2024"Day %d, ok"03/15/24ay 15, okDay 15, ok"zzz %Y"zz 2024zzz 2024"%Y-%m-%d end"2024-03-15 end2024-03-15 end(already correct)Dbecame%D=%m/%d/%y;zbecame%z= the UTC offset, empty for a tz-lessDatetime. In practice a format string could not begin with text —(strftime dt "Time: %H:%M")was silently wrong.Defect 2 — a format with no
%aborted the processmaster"hello"hello""Array_aset_BANG___int: Assertion 'n < a.len' failed.split-byreturns one segment, solnis0, and core'sArray.range 0 0 1(core/ArrayExt.carp:8) picks its comparison as(if (< start end) <= >=)— withstart == endthat is>=, which stays true as the counter climbs, so it writes past its one-element allocation.Upstream note: that is a bug in Carp core's
Array.range, not in this library, and it is not fixed here. The rewrite simply does not callArray.rangeat all, which removes the only trigger intime.Defect 3 —
Datetime.formatmixed byte and character indicesString.index-ofreturns a byte offset andString.char-atreads a byte, butString.prefix/suffixgo throughString.charsand count characters. So any non-ASCII literal before the%mis-sliced.This is reachable straight through
fmt, which handsformatone slice carrying a single directive plus its surrounding literal text:master(fmt "Größe: %Y" &dt)Größe: %Y2024Größe: 2024(Datetime.format "Größe: %Y" &dt)Größe: %Y2024Größe: 2024(Datetime.strftime &dt "Größe: %Y μs")2024röße: 2024 μsGröße: 2024 μs(Datetime.format &<3×0x80 bytes>"%Y" &dt)(Datetime.format "hello" &dt)Marellohelloindex-ofreturns byte 9 for"Größe: %Y", andString.prefix s 9then takes nine characters — the whole string. The abort isArray_unsafe_nth__Char: Assertion 'n < a.len' failedatcore/String.carp:128, hit when a format string's byte count exceeds its character count.formatkeeps its signature and its one-directive contract; it now slices withString.byte-slice. Two degenerate indices are guarded, becausebyte-slicedoes no bounds checking and a negative length wouldmemcpywithSIZE_MAX: a slice with no%and a slice ending in a lone%are returned unchanged. That also replaces the old"hello"→Marellobehaviour, which came from reading a specifier out of a slice that had none.What the rewrite is
One scanner over the format string by byte index — the same shape
expand-compound-formatsalready uses. Literal runs are flushed withString.byte-slicerather than(str (String.char-at ...)):char-atreturns a raw byte butChar.strUTF-8-encodes the codepoint, so byte0xC3would come back out as the two bytes0xC3 0x83. Slicing copies literals verbatim.format-forgets the same forwardregisterdeclarationformatalready had two lines above it, sostrftimecan dispatch to it directly without going through the interface.Behaviour changes, and the trailing-
%decisionThree
%-escape cases change. All three move toward C, verified against glibc'sstrftimewith the same fixture:master"%%Y"%2024%Y%Y"%%%Y"%%2024%2024%2024"%"%%"%Y%"20242024%2024%"%%"%%%"100%% done %Y"100% done 2024100% done 2024100% done 2024Trailing lone
%: it renders as a literal%. Three reasons —expand-compound-formatsin this same file already appends a trailing%verbatim, sostrftimeandstrptimenow agree on it; it is what glibc and Python do; and dropping it is silent data loss. Asserted in bothstrftimeandformat.One deliberate divergence from C is preserved: an unknown specifier renders as the character alone, so
"%Q"staysQ(glibc gives%Q). That is existing documented-by-behaviour semantics and the rewrite keeps it.Everything else is byte-for-byte identical. All 33 specifiers were dumped across three
Datetimes — with and without nanoseconds, with and without a timezone, spanning a leap year, a 53-week ISO year and a year boundary — 99 cells, of which 96 are unchanged and the 3 above are the only diffs. The compound%c/%x/%X/%D/%F/%R/%T/%r/%hexpansions are among the unchanged.Tests
15 new assertions in
test/time.carp, following the existingassert-equalstructure. Suite is 284 passed, 0 failed.13 of the 15 fail on unmodified
master— 11 produce wrong output, 2 abort the process. The other 2 (%%renders a literal percent, unknown specifier renders as the character) pass onmaster: they pin semantics that were already correct and that the rewrite had to preserve, so they are regression guards rather than bug reproductions. Flagging that explicitly rather than implying all 15 were failing.Scope
strptimeand its helpers (parse-digits,match-in-array,match-short-in-array, the%pbranch) have the same byte/character confusion on the input side —(Datetime.strptime <six 0x80 bytes> "%d %b %Y")also exits 134. That is deliberately not fixed here; it is a separate change on a separate code path. Noting it so it reads as seen, not missed.docs/Datetime.htmlcarries one line of drift fromgendocs.carp:strftime's inferred signature tightens from(Fn [a, (Ref String b)] String)to(Fn [(Ref Datetime a), (Ref String b)] String). The old body leftdtopen because it dispatched through theformatinterface; callingformat-fordirectly pins it toDatetime, which is what the doc string always claimed. Every call site in the repo already passes a&Datetime.No
CHANGELOG.mdwas added — this repo records releases in tag commit messages.Checks
carp -x test/time.carp→ 284 passed, 0 failedcarp -x gendocs.carp→ clean,docs/regenerated and committedcarp-fmt -c time.carp test/time.carp→ cleanangler time.carp/angler test/time.carp→ cleanOpened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.