Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tiny improvement of StringBuilder.Append(StringBuilder) #101020

Merged
merged 5 commits into from
Jun 5, 2024

Conversation

skyoxZ
Copy link
Contributor

@skyoxZ skyoxZ commented Apr 14, 2024

it is guaranteed that m_ChunkLength == 0 and m_ChunkChars.Length >= count after ExpandByABlock(count);.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 14, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 14, 2024
@jkotas jkotas added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 14, 2024
@danmoseley
Copy link
Member

Worth an assert to self document?

@skyoxZ
Copy link
Contributor Author

skyoxZ commented Apr 14, 2024

Worth an assert to self document?

My answer would rather be no. Perhaps ExpandByABlock 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 of minBlockCharCount.

/// <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>
private void ExpandByABlock(int minBlockCharCount)

@tannergooding
Copy link
Member

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.

@danmoseley
Copy link
Member

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.

@danmoseley danmoseley closed this May 14, 2024
@danmoseley danmoseley reopened this May 14, 2024
@danmoseley
Copy link
Member

timeouts, let's try again

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants