-
Notifications
You must be signed in to change notification settings - Fork 788
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
Add InplaceStringBuilder #157
Changes from 3 commits
7b5be3c
890d7d6
a207614
8c2b546
a00a5d5
29d128f
d2b4f00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Runtime.CompilerServices; | ||
|
||
namespace Microsoft.Extensions.Primitives | ||
{ | ||
public struct InplaceStringBuilder | ||
{ | ||
private int _length; | ||
private int _offset; | ||
private bool _writing; | ||
private string _value; | ||
|
||
public InplaceStringBuilder(int length) : this() | ||
{ | ||
_length = length; | ||
} | ||
|
||
public void IncrementLength(string s) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this + the next one really that useful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeps the symmetry, if you have char separator would be nice to have
vs
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But you can't copy paste it as easily ;) |
||
{ | ||
IncrementLength(s.Length); | ||
} | ||
|
||
public void IncrementLength(char c) | ||
{ | ||
IncrementLength(1); | ||
} | ||
|
||
public void IncrementLength(int length) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. Seems analogous to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's different because you are required to |
||
{ | ||
if (_writing) | ||
{ | ||
throw new InvalidOperationException("Cannot append lenght after write started."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cannot append |
||
} | ||
_length += length; | ||
} | ||
|
||
public unsafe void Append(string s) | ||
{ | ||
EnsureValue(s.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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you file a task for this instead? TODOs are easy to miss |
||
Unsafe.CopyBlock(value + _offset, pDomainToken, (uint)s.Length * 2); | ||
_offset += s.Length; | ||
} | ||
} | ||
public unsafe void Append(char c) | ||
{ | ||
EnsureValue(1); | ||
fixed (char* value = _value) | ||
{ | ||
value[_offset++] = c; | ||
} | ||
} | ||
|
||
private void EnsureValue(int length) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
if (_value == null) | ||
{ | ||
_writing = true; | ||
_value = new string('\0', _length); | ||
} | ||
if (_offset + length > _length) | ||
{ | ||
throw new InvalidOperationException($"Not enough space to write '{length}' characters, only '{_length - _offset}' left."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. space -> capacity |
||
} | ||
} | ||
|
||
// Debugger calls ToString so this method should be used to get formatted value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use |
||
public string Build() | ||
{ | ||
if (_offset != _length) | ||
{ | ||
throw new InvalidOperationException($"Entire reserved lenght was not used. Length: '{_length}', written '{_offset}'."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
return _value; | ||
} | ||
|
||
public override string ToString() | ||
{ | ||
// Clone string so we won't be modifying returned string if called before | ||
// whole value was written | ||
return new string(_value.ToCharArray()); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
using System; | ||
using Microsoft.Extensions.Primitives; | ||
using Xunit; | ||
|
||
namespace Microsoft.AspNetCore.Http.Tests.Internal | ||
{ | ||
public class InplaceStringBuilderTest | ||
{ | ||
[Fact] | ||
public void ToString_ReturnsStringWithAllAppendedValues() | ||
{ | ||
var s1 = "123"; | ||
var c1 = '4'; | ||
var s2 = "56789"; | ||
|
||
var formatter = new InplaceStringBuilder(); | ||
formatter.IncrementLength(s1); | ||
formatter.IncrementLength(c1); | ||
formatter.IncrementLength(s2); | ||
formatter.Append(s1); | ||
formatter.Append(c1); | ||
formatter.Append(s2); | ||
Assert.Equal("123456789", formatter.Build()); | ||
} | ||
|
||
[Fact] | ||
public void Build_ThrowsIfNotEnoughWritten() | ||
{ | ||
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'."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo "length" |
||
} | ||
|
||
[Fact] | ||
public void AppendLength_IfAppendWasCalled() | ||
{ | ||
var formatter = new InplaceStringBuilder(3); | ||
formatter.Append("123"); | ||
|
||
var exception = Assert.Throws<InvalidOperationException>(() => formatter.IncrementLength(1)); | ||
Assert.Equal(exception.Message, "Cannot append lenght after write started."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo |
||
} | ||
|
||
[Fact] | ||
public void Append_ThrowsIfNotEnoughSpace() | ||
{ | ||
var formatter = new InplaceStringBuilder(1); | ||
|
||
var exception = Assert.Throws<InvalidOperationException>(() => formatter.Append("123")); | ||
Assert.Equal(exception.Message, "Not enough space to write '3' characters, only '1' left."); | ||
} | ||
|
||
[Fact] | ||
public void ToString_ReturnsPartialyFormatedValue() | ||
{ | ||
var formatter = new InplaceStringBuilder(5); | ||
formatter.Append("123"); | ||
|
||
Assert.Equal("123\0\0", formatter.ToString()); | ||
} | ||
|
||
[Fact] | ||
public void ToString_ReturnedValueIsNotModified() | ||
{ | ||
var formatter = new InplaceStringBuilder(5); | ||
formatter.Append("123"); | ||
|
||
var s = formatter.ToString(); | ||
Assert.Equal("123\0\0", s); | ||
|
||
formatter.Append("45"); | ||
Assert.Equal("123\0\0", s); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benaadams why would it cause stackoverflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;-)