Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure that INumberBase implements IUtf8SpanFormattable #88840

Merged
merged 5 commits into from
Jul 14, 2023
Merged

Ensure that INumberBase implements IUtf8SpanFormattable #88840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe94d1b
Ensure that INumberBase implements IUtf8SpanFormattable
tannergooding Jul 13, 2023
183c7b2
Ensure we return the rented buffers in the IUtf8SpanFormattable.TryFo…
tannergooding Jul 13, 2023
fc54f30
Remember to slice the utf16Destination buffer and ensure we throw if …
tannergooding Jul 13, 2023
0c8d6b3
Update src/libraries/System.Private.CoreLib/src/System/Numerics/INumb…
tannergooding Jul 14, 2023
9ab1604
Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
tannergooding Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2608,6 +2608,9 @@
<data name="InvalidOperation_HandleIsNotPinned" xml:space="preserve">
<value>Handle is not pinned.</value>
</data>
<data name="InvalidOperation_InvalidUtf8" xml:space="preserve">
<value>Formatted string contains characters not representable as valid UTF-8.</value>
</data>
<data name="InvalidOperation_HashInsertFailed" xml:space="preserve">
<value>Hashtable insert failed. Load factor too high. The most common cause is multiple threads writing to the Hashtable simultaneously.</value>
</data>
Expand Down
59 changes: 57 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Numerics/INumberBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public interface INumberBase<TSelf>
ISubtractionOperators<TSelf, TSelf, TSelf>,
IUnaryPlusOperators<TSelf, TSelf>,
IUnaryNegationOperators<TSelf, TSelf>,
IUtf8SpanFormattable,
IUtf8SpanParsable<TSelf>
where TSelf : INumberBase<TSelf>?
{
Expand Down Expand Up @@ -297,7 +298,7 @@ static virtual TSelf Parse(ReadOnlySpan<byte> utf8Text, NumberStyles style, IFor
if (textMaxCharCount < 256)
{
utf16TextArray = null;
utf16Text = stackalloc char[512];
utf16Text = stackalloc char[256];
}
else
{
Expand Down Expand Up @@ -425,7 +426,7 @@ static virtual bool TryParse(ReadOnlySpan<byte> utf8Text, NumberStyles style, IF
if (textMaxCharCount < 256)
{
utf16TextArray = null;
utf16Text = stackalloc char[512];
utf16Text = stackalloc char[256];
}
else
{
Expand Down Expand Up @@ -456,6 +457,60 @@ static virtual bool TryParse(ReadOnlySpan<byte> utf8Text, NumberStyles style, IF
return succeeded;
}

bool IUtf8SpanFormattable.TryFormat(Span<byte> utf8Destination, out int bytesWritten, ReadOnlySpan<char> format, IFormatProvider? provider)
{
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
char[]? utf16DestinationArray;
scoped Span<char> utf16Destination;
int destinationMaxCharCount = Encoding.UTF8.GetMaxCharCount(utf8Destination.Length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would just use utf8Destination.Length directly for this. That is, just assume ASCII to simplify things. You're not going to be able to tell how many UTF-8 bytes the intermediate buffer will transcode to until after the intermediate buffer has already been populated, so...meh.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, its not strictly "needed" and in the 99% case won't even be different since it really only is length + 1 and check for overflow.

But, its nice to be consistent with the generally "correct" pattern and helps cover any edges that might creep in for user code. Our own APIs won't be hitting the DIM anyways.


if (destinationMaxCharCount < 256)
{
utf16DestinationArray = null;
utf16Destination = stackalloc char[256];
}
else
{
utf16DestinationArray = ArrayPool<char>.Shared.Rent(destinationMaxCharCount);
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
utf16Destination = utf16DestinationArray.AsSpan(0, destinationMaxCharCount);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: No slice is needed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not "needed", but its nice to be consistent with the general pattern that is needed in most algorithms. It helps avoid copy/paste or refactoring errors.

It also helps limit the amount of work done if the utf8Buffer would've been "too small" and the rented buffer was much larger, for whatever reason (that is, it allows faster and more consistent failure).

}

if (!TryFormat(utf16Destination, out int charsWritten, format, provider))
{
if (utf16DestinationArray != null)
{
// Return rented buffers if necessary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, I don't think these comments add much

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s a copy/paste of the same comment we have throughout similar code. I agree it probably doesn’t add much, but it’s consistent atm

ArrayPool<char>.Shared.Return(utf16DestinationArray);
}

bytesWritten = 0;
return false;
}

// Make sure we slice the buffer to just the characters written
utf16Destination = utf16Destination.Slice(0, charsWritten);

OperationStatus utf8DestinationStatus = Utf8.FromUtf16(utf16Destination, utf8Destination, out _, out bytesWritten, replaceInvalidSequences: false);
tannergooding marked this conversation as resolved.
Show resolved Hide resolved

if (utf16DestinationArray != null)
{
// Return rented buffers if necessary
ArrayPool<char>.Shared.Return(utf16DestinationArray);
}

if (utf8DestinationStatus == OperationStatus.Done)
{
return true;
}

if (utf8DestinationStatus != OperationStatus.DestinationTooSmall)
{
ThrowHelper.ThrowInvalidOperationException_InvalidUtf8();
}

bytesWritten = 0;
return false;
}

static TSelf IUtf8SpanParsable<TSelf>.Parse(ReadOnlySpan<byte> utf8Text, IFormatProvider? provider)
{
// Convert text using stackalloc for <= 256 characters and ArrayPool otherwise
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,12 @@ internal static void ThrowArraySegmentCtorValidationFailedExceptions(Array? arra
throw GetArraySegmentCtorValidationFailedException(array, offset, count);
}

[DoesNotReturn]
internal static void ThrowInvalidOperationException_InvalidUtf8()
{
throw new InvalidOperationException(SR.InvalidOperation_InvalidUtf8);
}

[DoesNotReturn]
internal static void ThrowFormatException_BadFormatSpecifier()
{
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10793,7 +10793,7 @@ public partial interface IMultiplyOperators<TSelf, TOther, TResult> where TSelf
static virtual TResult operator checked *(TSelf left, TOther right) { throw null; }
static abstract TResult operator *(TSelf left, TOther right);
}
public partial interface INumberBase<TSelf> : System.IEquatable<TSelf>, System.IFormattable, System.IParsable<TSelf>, System.ISpanFormattable, System.ISpanParsable<TSelf>, System.Numerics.IAdditionOperators<TSelf, TSelf, TSelf>, System.Numerics.IAdditiveIdentity<TSelf, TSelf>, System.Numerics.IDecrementOperators<TSelf>, System.Numerics.IDivisionOperators<TSelf, TSelf, TSelf>, System.Numerics.IEqualityOperators<TSelf, TSelf, bool>, System.Numerics.IIncrementOperators<TSelf>, System.Numerics.IMultiplicativeIdentity<TSelf, TSelf>, System.Numerics.IMultiplyOperators<TSelf, TSelf, TSelf>, System.Numerics.ISubtractionOperators<TSelf, TSelf, TSelf>, System.Numerics.IUnaryNegationOperators<TSelf, TSelf>, System.Numerics.IUnaryPlusOperators<TSelf, TSelf>, System.IUtf8SpanParsable<TSelf> where TSelf : System.Numerics.INumberBase<TSelf>?
public partial interface INumberBase<TSelf> : System.IEquatable<TSelf>, System.IFormattable, System.IParsable<TSelf>, System.ISpanFormattable, System.ISpanParsable<TSelf>, System.Numerics.IAdditionOperators<TSelf, TSelf, TSelf>, System.Numerics.IAdditiveIdentity<TSelf, TSelf>, System.Numerics.IDecrementOperators<TSelf>, System.Numerics.IDivisionOperators<TSelf, TSelf, TSelf>, System.Numerics.IEqualityOperators<TSelf, TSelf, bool>, System.Numerics.IIncrementOperators<TSelf>, System.Numerics.IMultiplicativeIdentity<TSelf, TSelf>, System.Numerics.IMultiplyOperators<TSelf, TSelf, TSelf>, System.Numerics.ISubtractionOperators<TSelf, TSelf, TSelf>, System.Numerics.IUnaryNegationOperators<TSelf, TSelf>, System.Numerics.IUnaryPlusOperators<TSelf, TSelf>, System.IUtf8SpanFormattable, System.IUtf8SpanParsable<TSelf> where TSelf : System.Numerics.INumberBase<TSelf>?
{
static abstract TSelf One { get; }
static abstract int Radix { get; }
Expand Down Expand Up @@ -10835,6 +10835,7 @@ static virtual TSelf CreateTruncating<TOther>(TOther value)
static virtual TSelf Parse(System.ReadOnlySpan<byte> utf8Text, System.Globalization.NumberStyles style, System.IFormatProvider? provider) { throw null; }
static abstract TSelf Parse(System.ReadOnlySpan<char> s, System.Globalization.NumberStyles style, System.IFormatProvider? provider);
static abstract TSelf Parse(string s, System.Globalization.NumberStyles style, System.IFormatProvider? provider);
bool System.IUtf8SpanFormattable.TryFormat(System.Span<byte> utf8Destination, out int bytesWritten, System.ReadOnlySpan<char> format, System.IFormatProvider? provider) { throw null; }
static TSelf System.IUtf8SpanParsable<TSelf>.Parse(System.ReadOnlySpan<byte> utf8Text, System.IFormatProvider? provider) { throw null; }
static bool System.IUtf8SpanParsable<TSelf>.TryParse(System.ReadOnlySpan<byte> utf8Text, System.IFormatProvider? provider, [System.Diagnostics.CodeAnalysis.MaybeNullWhen(false)] out TSelf result) { throw null; }
protected static abstract bool TryConvertFromChecked<TOther>(TOther value, [System.Diagnostics.CodeAnalysis.MaybeNullWhen(false)] out TSelf result)
Expand Down
Loading