Skip to content

measure text strings by byte length in maybe_stringref#314

Merged
agronholm merged 6 commits into
agronholm:masterfrom
sahvx655-wq:stringref-text-byte-length
Jun 28, 2026
Merged

measure text strings by byte length in maybe_stringref#314
agronholm merged 6 commits into
agronholm:masterfrom
sahvx655-wq:stringref-text-byte-length

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

String reference threshold counts code points, not encoded bytes

While following up on the namespace fix in #313 I lined maybe_stringref up against the decoder and found a second place they disagree that the threshold table does not cover: the encoder decides whether to register a string with value.len(), which for a str is the number of code points, while the length written on the wire and the decoder both count UTF-8 bytes. For any non-ASCII text the two sides then make opposite decisions about crossing the 3/4/5/7/11-byte threshold, the namespace indices drift apart, and subsequent tag 25 references resolve to the wrong earlier string, so the payload comes back silently corrupted instead of erroring. The text branch now measures UTF-8 byte length to match the decoder and the encoded form; bytestrings were already counted in bytes. The added regression round-trips a non-ASCII payload that previously decoded incorrectly.

@coveralls

coveralls commented Jun 8, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 94.674%. first build — sahvx655-wq:stringref-text-byte-length into agronholm:master

@agronholm agronholm left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@agronholm agronholm merged commit aeff00b into agronholm:master Jun 28, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants