-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Expose/test Span-based Convert methods #24474
Conversation
@@ -191,6 +191,7 @@ public static partial class Convert | |||
public static int ToBase64CharArray(byte[] inArray, int offsetIn, int length, char[] outArray, int offsetOut, Base64FormattingOptions options) { throw null; } | |||
public static string ToBase64String(byte[] inArray, Base64FormattingOptions options) { throw null; } | |||
public static string ToBase64String(byte[] inArray, int offset, int length, Base64FormattingOptions options) { throw null; } | |||
public static string ToBase64String(System.ReadOnlySpan<byte> bytes, Base64FormattingOptions options = Base64FormattingOptions.None) { throw null; } |
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.
Is this done in corert as well? If not then it will fail on uap and uapaot builds.
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.
Convert.cs is in the shared partition. Once it's merged into coreclr, it'll be mirrored over to corert.
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.
I see, that's great! I missed that part on your coreclr PR :)
@@ -38,6 +38,7 @@ | |||
<ItemGroup Condition="'$(TargetGroup)'!='netstandard'"> | |||
<Compile Include="System\BitConverterSpan.cs" /> | |||
<Compile Include="System\BitConverter.netcoreapp.cs" /> | |||
<Compile Include="System\Convert.netcoreapp.cs" /> |
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.
NIT: This will also run on UWP but I guess we don't have a good naming conventions for non-netstandard stuff
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.
Yeah
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.
couple comments but this looks good once your coreclr PR is merged and CI passes. Thanks for doing it Steve!
@dotnet-bot test this please |
@dotnet-bot test this please |
Commit migrated from dotnet/corefx@602d2ed
Depends on dotnet/coreclr#14358
Fixes https://github.com/dotnet/corefx/issues/22417
cc: @AlexGhiondea, @joperezr