Tiny improvement of StringBuilder.Append(StringBuilder)

[skyoxZ]

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

[danmoseley]

Worth an assert to self document?

[skyoxZ]

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.

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

Lines 2438 to 2446 in fa1164c

	/// <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]

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]

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]

timeouts, let's try again