NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

Tiny improvement of`StringBuilder.Append(StringBuilder)`#101020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

tannergooding merged 5 commits intodotnet:mainfromskyoxZ:patch-1

Jun 5, 2024

Merged

Tiny improvement of`StringBuilder.Append(StringBuilder)`#101020

tannergooding merged 5 commits intodotnet:mainfromskyoxZ:patch-1

Jun 5, 2024

Conversation

Copy link

Contributor

skyoxZ commentedApr 14, 2024

it is guaranteed thatm_ChunkLength == 0 andm_ChunkChars.Length >= count afterExpandByABlock(count);.

Update StringBuilder.cs

fcc0738

ghost added the needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners label

Apr 14, 2024

dotnet-policy-servicebot added the community-contributionIndicates that the PR has been added by a community member label

Apr 14, 2024

jkotas added area-System.Runtime and removed needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners labels

Apr 14, 2024

build-analysisbot mentioned this pull request

Apr 14, 2024

[browser]Process terminated assert handling at icall.c:6162#100249

Closed

Copy link

Member

danmoseley commentedApr 14, 2024

Worth an assert to self document?

Copy link

ContributorAuthor

My answer would rather be no. PerhapsExpandByABlock had been something more complicated but at the moment its responsibility is quite straightforward and well documented: asserting current chunk is full and moving to a new chunk with a minimum size ofminBlockCharCount.

runtime/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs

Lines 2438 to 2446 infa1164c

	/// <summary>
	/// Transfers the character buffer from this chunk to a new chunk, and allocates a new buffer with a minimum size for this chunk.
	/// </summary>
	/// <param name="minBlockCharCount">The minimum size of the new buffer to be allocated for this chunk.</param>
	/// <remarks>
	/// This method requires that the current chunk is full. Otherwise, there's no point in shifting the characters over.
	/// It also assumes that 'this' is the last chunk in the linked list.
	/// </remarks>
	privatevoidExpandByABlock(intminBlockCharCount)

Copy link

Member

tannergooding commentedApr 18, 2024

If we're making an assumption, we should add an assert to validate that assumption holds true long term. Otherwise, it is trivially possible for a bug to be introduced due to a later refactoring or change.

Copy link

Member

danmoseley commentedApr 18, 2024

Also relevant, in some places StringBuilder relies on use of unsafe code such that bounds mistakes could cause and in the past have caused heap corruption. It's another reason to assert assumptions a little more than you might in some other code.