Add StringBuilder Span-based APIs #22852
Conversation
|
@kookiz thanks for your contribution! I sent you collaborator invite, so that we can assign work items to you in future. Once you accept, I recommend to disable CoreFX notifications, otherwise you will be DoS'd. @stephentoub what is the PR blocked on? API review? Something else? |
@karelz, as the PR description says, this "depends on dotnet/coreclr#13163", which hasn't been merged yet. |
|
@kookiz, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
|
@dotnet-bot test this please |
|
@kookiz it is pointing out that you added types that aren't in UAP yet. I see the changes got into the coreRT repo in 3390e457df151e8481b63197b91f0f0b147c70ab so we just need to wait a day or two for corefx to see them, then we can reset this CI. It is also possible to update system.runtime\src\apicompatbaseline.* but no point just for a day or two. |
FYI, @danmosemsft, there's still a manual step involved... someone needs to kick off the mirror that pushes corert to the internal N branch. |
| @@ -7054,6 +7054,7 @@ public sealed partial class StringBuilder : System.Runtime.Serialization.ISerial | |||
| public System.Text.StringBuilder Append(uint value) { throw null; } | |||
| [System.CLSCompliantAttribute(false)] | |||
| public System.Text.StringBuilder Append(ulong value) { throw null; } | |||
| public System.Text.StringBuilder Append(ReadOnlySpan<char> value) { throw null; } | |||
There was a problem hiding this comment.
Nit: generally we use the full namespace, so System.ReadOnlySpan rather than just ReadOnlySpan. This applies to the other cases as well.
| [InlineData("Hello", new char[] { 'a' }, "Helloa")] | ||
| [InlineData("Hello", new char[] { 'b', 'c', 'd' }, "Hellobcd")] | ||
| [InlineData("", new char[] { 'e', 'f', 'g' }, "efg")] | ||
| [InlineData("Hello", new char[0], "Hello")] |
There was a problem hiding this comment.
Might be worthwhile adding a case that has a '\0' in the middle of it.
| var builder = new StringBuilder(original); | ||
| builder.Append(new ReadOnlySpan<char>(value)); | ||
| Assert.Equal(expected, builder.ToString()); | ||
| Assert.Equal(expected, builder.ToString()); |
There was a problem hiding this comment.
What's the reason for the duplicated line? Validating that ToString is idempotent? I'd hope that'd be covered in some test elsewhere, in which case it shouldn't also be necessary in this test focused on Append(Span)
There was a problem hiding this comment.
I wish it was something so smart, but I'm afraid I just slipped on my keyboard
|
It is in the N branch. |
|
should work now #23092 @dotnet-bot test UWP NETNative x86 Release Build please however to fix the netfx failure you will need to make the tests exclude netfx System\Text\StringBuilderTests.cs(1042,21): error CS7036: There is no argument given that corresponds to the required formal parameter 'count' of 'StringBuilder.CopyTo(int, char[], int, int)' [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Runtime\tests\System.Runtime.Tests.csproj]
|
|
@dotnet-bot test Packaging All Configurations x64 Debug Build please |
The tests will actually need to not be built in to the netfx test library. They should be moved to https://github.com/KooKiz/corefx/blob/c76b0dd4876ad21b7d4971e9f2bbe47ad20975e6/src/System.Runtime/tests/System/Text/StringBuilderTests.netcoreapp.cs. |
|
Better still. |
* Add StringBuilder Span-based APIs * Address PR feedback * Move tests to netcoreapp Commit migrated from dotnet/corefx@e2262cc
Contributes to https://github.com/dotnet/corefx/issues/22430
Depends on dotnet/coreclr#13163