Add StringBuilder.MoveChunks(StringBuilder) API#127823
Conversation
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f88c747f-738f-4828-b2b4-e5a1206af008 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f88c747f-738f-4828-b2b4-e5a1206af008 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/91a900e5-c44d-4d1f-8e7a-cec9c663fb20 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/72dddfc9-8dcf-4e53-8739-c78a8a0240d5 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new public StringBuilder.MoveChunks(StringBuilder source) API to enable O(1) transfer of an existing StringBuilder’s chunk chain to a new StringBuilder, leaving the source drained.
Changes:
- Added
StringBuilder.MoveChunks(StringBuilder)implementation inSystem.Private.CoreLib. - Updated the
System.Runtimereference assembly to expose the new API. - Added new
StringBuilderTestscoverage for null handling, empty/single/multi-chunk transfers, and post-drain behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs |
Adds the MoveChunks API and drains the source instance after transferring chunk state. |
src/libraries/System.Runtime/ref/System.Runtime.cs |
Adds the public ref signature for StringBuilder.MoveChunks. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/StringBuilderTests.cs |
Adds tests for chunk transfer behavior and drained-source behavior (including reflection-based validation). |
tarekgh
left a comment
There was a problem hiding this comment.
Left minor comments, LGTM, otherwise.
One last question, is there any thread safety worries? I mean the source chunks get mutated while moving?
…r review feedback - Remove `source.m_MaxCapacity = 0` so the drained source remains fully usable - Update XML docs to clarify source is empty but usable with MaxCapacity preserved - Update multi-chunk test to use MemoryMarshal.TryGetArray for backing-array identity checks - Replace MoveChunks_DrainedSourceIsUnusable with MoveChunks_DrainedSource_RemainsUsable - Add MoveChunks_AlreadyDrainedSource_ProducesEmptyDestination test - Update AssertSourceIsDrained to remove MaxCapacity==0 assertion - Add null-forgiving operators to s_chunkCharsField reflection call - Add System.Runtime.InteropServices using directive for MemoryMarshal Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/7936562a-e7c5-4539-885e-7d1582969b64 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
|
@tarekgh, no more than any type has. Most types are not explicitly thread safe and so require some kind of using lock if they are expecting multithreaded access to be going on. |
|
/ba-g net security tests failed with null log |
Implements the approved API from #97570, enabling O(1) transfer of a
StringBuilder's chunk chain to a fresh instance so that callers (e.g. Roslyn'sStringBuilderText) can hold a no-copy, immutable view of the contents while releasing the original.Description
StringBuilder.MoveChunks(src/libraries/System.Private.CoreLib/.../StringBuilder.cs)StringBuildervia the existing privateStringBuilder(StringBuilder from)copy constructor, which transfersm_ChunkChars,m_ChunkPrevious,m_ChunkLength,m_ChunkOffset, andm_MaxCapacityfromsourceand avoids the wasted 16-char default buffer thatnew StringBuilder()would allocate.sourceby zeroingm_ChunkChars(set toArray.Empty<char>()),m_ChunkPrevious,m_ChunkLength, andm_ChunkOffset, while preservingm_MaxCapacity. The drainedsourceis left in an empty but fully usable state — distinct fromClear(), which retains the buffer. Subsequent append or insert operations onsourcewill succeed, allocating new buffers as needed.ArgumentNullExceptionwhensourceisnull.System.Runtime.cs).StringBuilderTestscover null arg, empty source, single chunk, multi-chunk chain (usingMemoryMarshal.TryGetArrayto assert backing-array identity by reference, not just content), callingMoveChunkson an already-drained source, and that the drained source remains usable with its originalMaxCapacitypreserved.Customer Impact
Unblocks the no-copy
SourceTextshape Roslyn has been waiting on (dotnet/roslyn#61326) and provides a general primitive for transferring ownership of aStringBuilder's contents without materializing a contiguous string or array of chunks.Regression?
New API; no behavior change to existing members.
Risk
Pure addition; the implementation only manipulates internal fields already reachable via existing code paths. After the call,
sourceretains its originalMaxCapacityand is fully usable as an emptyStringBuilder.