feat: FixedSizeValueStringBuilder (closes stackalloc-based version - #315
Conversation
19908f8 to
74efa03
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A compile-blocking handler issue and unresolved implementation and documentation findings remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds a fixed-capacity, stack-backed FixedSizeValueStringBuilder with overflow handling, interpolation support, tests, benchmarks, and documentation.
Changes:
- Implements fixed-size append and formatting behavior.
- Adds migration to the growable builder.
- Adds tests, benchmarks, documentation, and changelog updates.
| File | Reviewed changes and final findings |
|---|---|
tests/LinkDotNet.StringBuilder.UnitTests/FixedSizeValueStringBuilder.Tests.cs |
Core behavior tests. |
tests/LinkDotNet.StringBuilder.UnitTests/FixedSizeValueStringBuilder.InterpolatedStringHandler.Tests.cs |
Interpolated-string handler tests. |
tests/LinkDotNet.StringBuilder.Benchmarks/StackAllocVsRentBenchmark.cs |
Stack versus pooled allocation benchmark. |
tests/LinkDotNet.StringBuilder.Benchmarks/PaddingBenchmark.cs |
Padding benchmark. |
tests/LinkDotNet.StringBuilder.Benchmarks/FormatComparisonBenchmark.cs |
Formatting comparison benchmark. |
tests/LinkDotNet.StringBuilder.Benchmarks/FixedSizeBenchmark.cs |
Fixed-size performance benchmark. |
src/LinkDotNet.StringBuilder/ValueStringBuilder.cs |
Buffer-adopting constructor support. |
src/LinkDotNet.StringBuilder/FixedSizeValueStringBuilder.InterpolatedStringHandler.cs |
Critical (1): the default argument creates an ambiguous overload call; make its target type explicit. |
src/LinkDotNet.StringBuilder/FixedSizeValueStringBuilder.cs |
Moderate (2): indexer exposes unwritten buffer contents. Moderate (3): unconstrained generic ISpanFormattable checks can box value types.Moderate (1): formatted IFormattable values can lose their supplied format. |
README.md |
Nit (1): scope the zero-allocation heading to the backing buffer or pooled-rent behavior. |
docs/site/articles/toc.yml |
Documentation navigation update. |
docs/site/articles/known_limitations.md |
Updated limitations. |
docs/site/articles/fixed_size.md |
Nit (1): qualify allocation guarantees. Nit (1): handle overflow before moving to the growable builder. Nit (1): document interpolated-handler per-part behavior. |
docs/site/articles/exceptions_and_edge_cases.md |
Moderate (1): align indexer bounds documentation with the implementation. |
docs/site/articles/concepts.md |
Documentation metadata update. |
docs/site/articles/comparison.md |
Nit (1): do not claim capacity checks were removed. |
docs/site/articles/best_practices.md |
Usage recommendations. |
docs/site/articles/additional_members.md |
Nit (1): correct the Rune overload documentation for span values. |
CHANGELOG.md |
Release notes update. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// <returns><see langword="true"/> if it fit; otherwise, <see langword="false"/>, which makes the compiler | ||
| /// skip the rest of the interpolated string.</returns> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public bool AppendFormatted<T>(T value) => AppendFormatted(value, default); |
| // The cast to ISpanFormattable reads like a box, but because the interface method is invoked directly on the | ||
| // cast expression the JIT emits a constrained call and elides the allocation for value types. Hoisting it into | ||
| // an ISpanFormattable local, or routing it through a T : ISpanFormattable helper, would box for real and break | ||
| // this type's zero-allocation guarantee. Same shape the BCL uses in DefaultInterpolatedStringHandler. | ||
| if (value is ISpanFormattable) |
4cba116 to
288287a
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Ownership aliasing, incomplete interpolation support, and inaccurate allocation and benchmark claims need correction.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (14)
Document unique ownership requirements after moving the builder · New Disambiguate default format overload Add alignment overloads to the interpolation handler · New Avoid double boxing in interpolated formatting · New Avoid boxing interpolation holes Narrow changelog claim about heap allocation · New Clarify that zero allocation only covers backing storage · New Reframe allocation guarantee as fixed backing storage · New Fix benchmark output mismatch before regenerating results · New Correct documentation of indexer bounds behavior · New Narrow the zero-allocation guarantee · New Document unique ownership for moved buffers · New Correct public allocation guarantee summary · New Remove extra benchmark space for equivalent workloads · New
Resolved since last review (1)
| buffer = default; | ||
| bufferPosition = 0; | ||
| overflowed = true; |
| public bool AppendFormatted<T>(T value) => Builder.TryAppendFormatted(value, default); | ||
|
|
||
| /// <summary> | ||
| /// Appends a formatted value to the handler. | ||
| /// </summary> | ||
| /// <param name="value">The value to format.</param> | ||
| /// <param name="format">The format string.</param> | ||
| /// <typeparam name="T">The type of the value.</typeparam> | ||
| /// <returns><see langword="true"/> if it fit; otherwise, <see langword="false"/>, which makes the compiler | ||
| /// skip the rest of the interpolated string.</returns> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public bool AppendFormatted<T>(T value, string? format) => AppendFormatted(value, format.AsSpan()); |
| if (value is ISpanFormattable) | ||
| { | ||
| if (overflowed) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (!((ISpanFormattable)value).TryFormat(buffer[bufferPosition..], out var written, format, null)) |
|
|
||
| ### Added | ||
|
|
||
| - `FixedSizeValueStringBuilder`: a non-growing `ref struct` string builder backed by a caller-supplied buffer that never allocates on the heap. Appends are atomic and the first one that does not fit latches `Overflowed`, which `ClearOverflow` resets. |
| ``` | ||
| Note that this will prevent you from returning `stringBuilder` or assigning it to an `out` parameter. | ||
|
|
||
| ### Guaranteed zero allocation with `FixedSizeValueStringBuilder` |
| Reading members never throw either: `AsSpan()`, `ToString()` and the indexer all see only the characters that were | ||
| actually written, and `TryCopyTo` returns `false` rather than throwing when the destination is too small. |
| Both builders would otherwise write into the same memory, so the move **consumes the source**. What is left behind is | ||
| an empty builder with `Capacity` of zero and `Overflowed` set to `true`. Reading it is safe and any further append is | ||
| a no-op, so a stale use cannot corrupt the buffer its new owner is writing into: |
| namespace LinkDotNet.StringBuilder; | ||
|
|
||
| /// <summary> | ||
| /// A string builder backed by a fixed-size, caller-supplied buffer which never grows and never allocates on the heap. |
6c9593a to
09526fb
Compare



Fixes #264