[PatrickHaecker]

The string types currently are inconsistent. The definitions which seem to make most sense:

1. StaticString{N}, with a capacity of N UTF-8 code units, uses all N code units as logical content, including all NULs and any constructor-added padding.
2. SubStaticString{N,R}, with integer unit range type R, has exactly the bytes selected by its range (for example, 2:5), including any NULs.
3. CStaticString{N} ends before its first NUL, or after all N stored bytes if none is present. An extra safety NUL guarantees termination for C calls.
4. PaddedStaticString{N, PAD} excludes only trailing PAD bytes. Internal occurrences of PAD remain content. NULs remain content unless they occur at the end and PAD == 0.

This addresses the review finding from #28.

This build on #31, although quite some things had to be changed to be consistent.

[mkitti]

Something that @stevengj has mentioned in the past is that StaticStrings.jl may be trying to implement too many types and that perhaps we should reduce the number of types at some point. Perhaps this is a symptom of that issue. I would appreciate your perspective on that @PatrickHaecker .

Also, the group of pull requests seems to be complete now, so I can also consider all the changes in totality as well as individually. Let me know if there are more coming.

[Copilot]

Copilot review overview

🟡 Changes recommended

Removing the documented data accessor without a compatibility path introduces an avoidable downstream API break.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Preserve deprecated StaticStrings.data accessor for API compatibility · New

What changed in this PR

Aligns all static string types around consistent logical-content and backing-storage semantics, addressing the comparison inconsistency identified in #28.

Changes:

• Defines logical content rules across all four string types.
• Updates constructors, conversions, comparisons, indexing, pointers, and IO.
• Expands tests and documentation for the revised behavior.

File	Description
Project.toml	Adds Serialization test dependency.
README.md	Documents new content semantics and IO behavior.
docs/​src/​index.md	Updates API documentation and examples.
src/​AbstractStaticString.jl	Redefines capacity semantics and removes data.
src/​comparison.jl	Compares logical code units consistently.
src/​convert.jl	Converts logical content and adjusts pointers.
src/​io.jl	Writes logical bytes without implicit terminators.
src/​macros.jl	Adds inferred padded-literal capacity.
src/​strings.jl	Corrects validation and substring offsets.
src/​types.jl	Implements per-type logical-content boundaries.
test/​AbstractStaticString.jl	Tests code-unit and content boundaries.
test/​ambiguities.jl	Updates restricted range coverage.
test/​ccall.jl	Tests logical-content pointers.
test/​comparison.jl	Tests cross-type equality and ordering.
test/​construction.jl	Tests constructors, ranges, and suffix storage.
test/​content.jl	Adds cross-type consistency tests.
test/​convert.jl	Tests logical-content conversion.
test/​io.jl	Expands logical IO and serialization coverage.
test/​macros.jl	Tests inferred padded literals.
test/​post_julia_1_6/​comparison.jl	Updates comparison expectations.
test/​post_julia_1_6/​construction.jl	Updates padded construction tests.
test/​post_julia_1_6/​macros.jl	Extends allocation tests.
test/​runtests.jl	Includes the new content tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[PatrickHaecker]

Solved with the applied code suggestion below. Thanks!

[PatrickHaecker]

Something that @stevengj has mentioned in the past is that StaticStrings.jl may be trying to implement too many types and that perhaps we should reduce the number of types at some point. Perhaps this is a symptom of that issue. I would appreciate your perspective on that @PatrickHaecker .

Thanks, that is an interesting point. There definitely was some redundancy between StaticString and SubStaticString, but it was hard to grasp because the effective length was not used consistently (at least I haven't found a simple defining rule valid for all cases). But in the end both StaticString and SubStaticString could somehow provide a statically sized memory and use the first part of it.

That redundancy is gone with #32, so in that sense it is now fixed.

StaticString is the string which not only always has the same length, but where that length is known statically to the compiler, so it is perfect for loop-unrolling and vectorization.

SubStaticString is the workhorse for variable strings up to a certain (statically known) length. While you could get the same semantics and the same memory layout for a SubStaticString as for a StaticString with a clever singleton AbstractUnitRange (e.g. OneToEnd), I guess that the compiler could still not see that the logical length is known statically. In that sense, the two types are strictly different now.
(If starting from scratch I would probably now either rename SubStaticString into RangedStaticString or even rename the current SubStaticString into StaticString and the current StaticString into FixedStaticString, but that is naming, not redundancy.)

If you look at this pattern of "always use the full length" and "use a part of it", it becomes clear that CStaticString and PaddedStaticString do not completely, or at least not fully consistently, play on this axis. If the padding is a NUL character, CStaticString is kind-of the "always use the full length" type, except that it doesn't as the NUL can come earlier. PaddedStaticString is kind-of the "use a part of it", but you can't omit the first part, i.e. have a variable start defined by an index which ends by the padding.

Additionally, I am not sure whether it's really better to have a separate byte for the NUL in CStaticString (but not a separate padding in PaddedStaticString). I could also imagine to not have that separate byte and make sure by the accessors that the logical string just always ends with the NUL. This might be a bit slower (I haven't checked, but it sounds plausible), but it had the advantage of avoiding the inconsistency of having a variable NUL to then have some garbage to then have an additional guaranteed NUL.

So while there is some redundancy between CStaticString and PaddedStaticString on the one hand, not all possibly useful types are implemented on the other hand. At least if I'd start from scratch, I'd think about having different variants of PaddedStaticString

• always terminate the logical string
• explicit or implicit terminate (the current version)
• an extra termination PAD (maybe even a codepoint) potentially
  and then I'd remove CStaticString or make it an alias to one of the padded strings with NUL as the PAD.

I would then try a generic SubString form, probably similar to Base.SubString, but with an AbstractUnitRange as a second type parameter replacing offset::Int and ncodeunits::Int by ind::R as in SubStaticString. Thinking about it, this might become Base.RangedSubString, where Base.SubString is just an alias with UnitRange{Int} as the second parameter and using properties and constructors to keep compatibility with existing field accesses (edit: this brought up an idea how Julia could support changing SubString in a backward-compatible way). Then I would create SubString-forms and check whether these could both obsolete SubStaticString as well as allowing using an efficient substring of the then-padded strings.

What is still missing, but might be out-of-scope for a package named StaticStrings: A variable string up to a certain runtime-known length. So that N is a field, not a type parameter (as compared to SubStaticString) and that only a part of the memory can be used and reused (as compared to String). This concept might again be used for the other types, too.

Also, the group of pull requests seems to be complete now, so I can also consider all the changes in totality as well as individually. Let me know if there are more coming.

I have nothing more planned. Of course, something else might come up, but that's always the case, so nothing which should hold us back. It might indeed make sense to look at all the changes at once if that does not seem too large a task. I thought I had it relatively well-structured, but the review finding about the inconsistency lead to an unplanned and large PR which rewrote some things of previous PRs.

So just from reading the code it would be more efficient to just review #32 in total. But for really understanding the changes and discussing aspects this

is typically too large for an efficient discussion. I could ask an LLM to restructure the total in some new PRs, making sure that the overall diff stays the same, if that is helpful, but the additional branches and kind of losing the discussions would also have its price. So it's your call which workflow you prefer.

[mkitti]

Review of #32 — "Make types consistent"

Verdict up front: the direction is right, and the underlying model — backing capacity N versus logical content — is the correct fix for a genuine pre-existing mess. I verified the central claims against main rather than reading the diff alone; evidence below. I have one substantive question (write/read round-tripping for CStaticString), plus some decisions and notes.

What I verified independently

The "before" numbers below are measured on current main, not inferred.

1. This fixes a silent ==/hash invariant violation — unmentioned in the PR description, and the strongest argument for merging it.

On main today:

julia> CStaticString{10}("Hello") == "Hello"
true

julia> hash(CStaticString{10}("Hello")) == hash("Hello")
false

julia> d = Dict{Any,Int}(CStaticString{10}("Hello") => 1);

julia> haskey(d, "Hello")
false

The same holds for PaddedStaticString and SubStaticString: == reports equal, hash disagrees, so Dict/Set lookups silently miss. Any user putting these types in a hash-based container is affected. The cause is that == compares logical content while the hash path ends up over the full backing storage.

test/content.jl pins exactly this down — hash, Dict, Set, ==, isequal, cmp, isless against String/SubString references across all four types. That file is the best part of this PR.

2. It also fixes SubStaticString slicing — also unmentioned. On main:

julia> s = SubStaticString(Tuple(codeunits("xabc")), 2:4);   # content "abc"

julia> s[1:3]
"xab"     # wrong: the range offset is ignored

src/strings.jl now adds offset to the pointer_from_objref arithmetic, and @testset "UTF-8 index errors" covers it via SubStaticString("xαβ!", 2:5). Worth a release-note line, since it is a wrong-answer bug rather than an inconsistency.

3. The #28 finding is genuinely resolved. equal_codeunits no longer special-cases CStaticString via length(data(a)); == and cmp both use ncodeunits uniformly. The expectations in test/comparison.jl invert accordingly — e.g. (SubStaticString("a\0", 1:2), CStaticString("a\0")) goes from true to false — which is the correct consequence.

The one thing I would push back on: write/read no longer round-trip for CStaticString

First, what did not change, since the title invites the wrong conclusion: the in-memory safety NUL is still there. _nul::UInt8 is retained, CStaticString{N} still occupies N+1 bytes, and the new test/ccall.jl cases confirm strlen still works through both Cstring and Ptr{UInt8}. The type's purpose — zero-copy, guaranteed-terminated buffers for C — is intact.

What changed is the stream path: write(io, ::CStaticString) now emits ncodeunits bytes with no terminator. Measured on main:

write(io, cstatic"Hello")              -> 6 bytes,  "Hello\0"
write(io, CStaticString{10}("Hello"))  -> 10 bytes, "Hello\0\0\0\0\0"

After this PR both emit 5 bytes, "Hello".

The issue is that read(io, ::Type{CStaticString}) is unchanged — it still reads until the first NUL and consumes it. So our own write/read pair no longer round-trips:

# with this PR's write semantics
io = IOBuffer(); write(io, CStaticString("Hello")); seekstart(io)
read(io, CStaticString)    # EOFError

(Confirmed by feeding the post-PR byte sequence to the current read.)

The README handles this by directing users to write(io, s, '\0'), but I would rather the two halves of our own API agree by default. Options, roughly in my order of preference — interested in your view:

1. Keep write type-uniform as you have it, and make read(io, CStaticString) consistent with it (or deprecate the NUL-delimited read in favour of an explicit delimiter argument).
2. Keep the terminator in write(io, ::CStaticString) only — one type-specific asymmetry, in exchange for a round trip that works.
3. Keep as-is, add a round-trip helper, and document it prominently.

Was uniformity of write the deliberate priority over the round trip? If the intended contract is "write is content; Serialization is for round trips," that is defensible — but then read(io, CStaticString) consuming a delimiter it never writes is the odd one out.

Other breaking changes, for the record

Not objections, but they should be captured somewhere user-visible:

• StaticStrings.data is removed (also flagged by Copilot); Tuple(s) replaces it. Do you want a deprecated forwarding method for one cycle, or is a clean break fine at this stage?
• CStaticString no longer validates that NULs are terminal — _check_for_nuls is gone, so CStaticString("He\0llo") is now accepted (content "He", the rest dead storage) where it previously threw ArgumentError. This is "first NUL wins" C semantics and I think it is right, but it does silently accept data that used to be rejected.
• String(cstatic"Hello"10) changes from "Hello\0\0\0\0\0" to "Hello", and cstatic"Hello"10 == static"Hello"10 changes from true to false.
• Over-long inputs now throw InexactError instead of truncating silently (PaddedStaticString{4,'o'}("Hello"), and CStaticString{N} with M > N).
• PaddedStaticString{N,PAD}(::AbstractStaticString) is removed; the generic AbstractString path now copies logical content rather than backing bytes.

Smaller points

• Base.codeunits(::CStaticString) is now a runtime-length tuple slice (string.data[1:ncodeunits(string)]), so it allocates where it previously returned s.data directly. You documented that hazard on codeunits(::SubStaticString) — the same note belongs here and on PaddedStaticString.
• One deleted assertion I could not match to a replacement: @test_throws ArgumentError SubStaticString{5,UnitRange{Int}}(text, 1:6) in test/construction.jl. Does that now throw something else, or is it covered elsewhere? (The removed read(io, CStaticString{6}) == static"hello\0" family were updated in place, so those are clearly deliberate.)
• test/content.jl and test/AbstractStaticString.jl are missing a trailing newline.

Things I checked that are fine

Recording these so nobody re-litigates them:

• pointer_from_objref is safe for exotic range types. I suspected that SubStaticString{N,UnitRange{BigInt}} is not isbits, so Ref(s) would box and the new offset arithmetic in unsafe_convert would point at a pointer rather than at the bytes. It does not — Julia stores the immutable struct inline in the RefValue. I confirmed the leading bytes read back correctly for Int, UInt8 and BigInt ranges.
• byte_string_classify(Base.CodeUnits(s)) resolves and behaves; replacing the hand-rolled ccall(:u8_isvalid, ..., Ref(s), N) — which used N rather than ncodeunits — is a fix in its own right.
• The macro allocation tests were not dropped, only reordered, and padded gained one.

Overall I am in favour of the substance. The write/read question is the one I would like settled before merge; the rest are decisions or notes.

🤖 Generated with Claude Code

[mkitti]

Two specific suggestions, following up on my review above.

Housekeeping first: I pushed a merge of main into this branch to clear the conflicts left by the squash-merged #23 and #24. I verified the resulting tree is byte-identical to your version — the merge is pure history reconciliation, no content change. One thing to be aware of: the auto-merge wanted to splice a stale second copy of write_strings! and the old Allocation-free IO testset into test/io.jl; I dropped it in favour of your rewritten versions. Full suite passes locally, 2098/2098.

1. Please restore data as a deprecated method

Rather than removing StaticStrings.data outright, I would like it kept for one transition cycle. It was a documented accessor, and the cost is a single line. I left an inline suggestion below.

I verified the exact form behaves as intended:

• returns the same value as Tuple(s)
• stays unexported, as data always was (the trailing false)
• warns under --depwarn=yes, silent otherwise

No need to add it back to the docs index — deprecated is enough, and your Tuple docstring is the right place to point people.

2. eachindex — the issue is identified, and it is not what the comment claimed

The # eachindex is slower in Julia 1.13, so correct the index manually on access comment from #25 no longer exists in this PR, because Base.String is now unsafe_string(pointer, count). Nothing to change here — but I want the measurement on record, since the claim is still live in #25/#26/#27/#31 and shouldn't be carried forward.

I could not reproduce any eachindex penalty. Comparing all three strategies that appeared across the stack on identical inputs (backing tuple + offset + count), with BenchmarkTools on both 1.12.6 and 1.13.0-rc1:

Julia	N / offset / count	unsafe_string (this PR)	manual loop (#25–#31)	eachindex
1.12.6	8 / 1 / 4	21.7 ns	35.8 ns	35.2 ns
1.12.6	32 / 2 / 20	25.6 ns	43.5 ns	47.2 ns
1.12.6	64 / 0 / 64	32.7 ns	82.5 ns	70.7 ns
1.12.6	256 / 8 / 200	70.2 ns	179.5 ns	179.6 ns
1.13.0-rc1	8 / 1 / 4	21.6 ns	35.6 ns	35.3 ns
1.13.0-rc1	32 / 2 / 20	25.3 ns	43.3 ns	47.2 ns
1.13.0-rc1	64 / 0 / 64	32.0 ns	70.2 ns	82.1 ns
1.13.0-rc1	256 / 8 / 200	70.1 ns	181.0 ns	178.7 ns

Two conclusions:

1. Manual indexing and eachindex are indistinguishable — the gap changes sign between rows and between versions, which is noise, not a regression. Specifically there is no 1.13 penalty. If those PRs stay open, please drop the comment or attach a reproducer; if they are superseded by this one, it goes away on its own.
2. Your unsafe_string rewrite is well justified — 1.6x to 2.6x faster than either loop, consistently on both versions. That is a far stronger argument than the eachindex claim ever was, and it is worth putting in the PR description in its place.

So: nothing to do in #32 for this second point. It is an argument for the direction you took.

🤖 Generated with Claude Code

[PatrickHaecker]

Anything I should do here at the moment?

The write/read is indeed a delicate compromise and I do not have a lot of experience with both interfaces. I tried to find the most consistent definition based on my limited experience in that area, but I am not sure whether users have different expectations.

[mkitti]

I'm thinking about the right balance between speed and safety here.

The main todo at the moment is to bring back data() as a public but deprecated accessor that is just an alias for Tuple.

[PatrickHaecker]

The main todo at the moment is to bring back data() as a public but deprecated accessor that is just an alias for Tuple.

Are you sure that this is useful? It was never exported in StaticStrings.jl as far as I know. It seems to have been exported between 2022-09-27–2022-09-28 within NStrings, but I don't think that anyone depended on it within that single day. And even if they did, we have it back as a deprecated (internal) method thanks to your commit.

[mkitti]

I think the deprecated version is fine.

[PatrickHaecker]

So anything else I should support with at the moment?

[PatrickHaecker]

Thanks for all the reviews, the discussions and for merging the changes!