Add InplaceStringBuilder #157
Conversation
|
@benaadams can we keep using |
|
That runs Mono also doesn't it? Doesn't look like any platform uses the aligned cpu instructions currently 😁 |
|
Maybe add a |
| { | ||
| AppendLength(s.Length); | ||
| } | ||
| public void AppendLength(char c) |
| } | ||
| } | ||
|
|
||
| private void EnsureValue(int length) |
| fixed (char* value = _value) | ||
| fixed (char* pDomainToken = s) | ||
| { | ||
| //TODO: Use CopyBlockUnaligned when added https://github.com/dotnet/corefx/issues/12243 |
There was a problem hiding this comment.
Could you file a task for this instead? TODOs are easy to miss
| { | ||
| if (_writing) | ||
| { | ||
| throw new InvalidOperationException("Cannot append lenght after write started."); |
| } | ||
| } | ||
|
|
||
| // Debugger calls ToString so this method should be used to get formatted value |
There was a problem hiding this comment.
You can use DebuggerDisplayAttribute to control what gets displayed by the debugger.
| IncrementLength(1); | ||
| } | ||
|
|
||
| public void IncrementLength(int length) |
There was a problem hiding this comment.
Yeah. Seems analogous to StringBuilder in that case
There was a problem hiding this comment.
It's different because you are required to IncrementLength for all strings before appending them and I felt like having a method is less error prone then incrementing property.
| _length = length; | ||
| } | ||
|
|
||
| public void IncrementLength(string s) |
There was a problem hiding this comment.
Is this + the next one really that useful?
There was a problem hiding this comment.
Keeps the symmetry, if you have char separator would be nice to have
IncrementLength(Key);
IncrementLength(SeperatorChar);
IncrementLength(Value);
Append(Key);
Append(SeperatorChar);
Append(Value);
vs
IncrementLength(Key);
IncrementLength(1);
IncrementLength(Value);
Append(Key);
Append(SeperatorChar);
Append(Value);
There was a problem hiding this comment.
Capacity += Key.Length + 1 + Value.Length; is about as pretty :)
There was a problem hiding this comment.
But you can't copy paste it as easily ;)
| { | ||
| if (_offset != _length) | ||
| { | ||
| throw new InvalidOperationException($"Entire reserved lenght was not used. Length: '{_length}', written '{_offset}'."); |
| } | ||
| if (_offset + length > _length) | ||
| { | ||
| throw new InvalidOperationException($"Not enough space to write '{length}' characters, only '{_length - _offset}' left."); |
| { | ||
| throw new InvalidOperationException($"Entire reserved lenght was not used. Length: '{_length}', written '{_offset}'."); | ||
| throw new InvalidOperationException($"Entire reserved length was not used. Length: '{_capacity}', written '{_offset}'."); |
| namespace Microsoft.Extensions.Primitives | ||
| { | ||
| [DebuggerDisplay("Value = {_value}")] | ||
| public struct InplaceStringBuilder |
There was a problem hiding this comment.
Maybe a lil late, but we should add a comment that this should only be used for well-known reasonably sized strings. For everything else, use StringBuilder. (Primarily because user code would see this via HttpAbstractions \ Mvc)
There was a problem hiding this comment.
Add something scary but true? "otherwise it will cause a stackoverflow" maybe also "do not use across await points" though not sure it will let you anyway.
There was a problem hiding this comment.
@benaadams why would it cause stackoverflow?
There was a problem hiding this comment.
Sorry thought it was stackalloc'd, reading too many PRs. This wouldn't cause a stack overflow; though it might upset some people on the clr team ;-)
| var formatter = new InplaceStringBuilder(5); | ||
| formatter.Append("123"); | ||
| var exception = Assert.Throws<InvalidOperationException>(() => formatter.Build()); | ||
| Assert.Equal(exception.Message, "Entire reserved lenght was not used. Length: '5', written '3'."); |
| formatter.Append("123"); | ||
|
|
||
| var exception = Assert.Throws<InvalidOperationException>(() => formatter.IncrementLength(1)); | ||
| Assert.Equal(exception.Message, "Cannot append lenght after write started."); |
…master [automated] Merge branch 'release/2.2' => 'master'
ghost
locked as resolved and limited conversation to collaborators
Migrated from: aspnet/HttpAbstractions#717
Added
.Build()method for better debugging experience.Intended to be used instead of pooled StringBuilder or string.Concat when all parts of string are known. (Use cases: aspnet/HttpAbstractions#699, aspnet/HttpAbstractions#716)
Does only 1 allocation of resulting string.
@benaadams @davidfowl @Tratcher @halter73 @rynowak