-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Ensure that INumberBase implements IUtf8SpanFormattable #88840
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsSince
|
else | ||
{ | ||
utf16DestinationArray = ArrayPool<char>.Shared.Rent(destinationMaxCharCount); | ||
utf16Destination = utf16DestinationArray.AsSpan(0, destinationMaxCharCount); |
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: No slice is needed here.
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.
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).
…we couldn't transcode back to valid UTF-8 in the DIM
{ | ||
char[]? utf16DestinationArray; | ||
scoped Span<char> utf16Destination; | ||
int destinationMaxCharCount = Encoding.UTF8.GetMaxCharCount(utf8Destination.Length); |
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: 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.
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.
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.
src/libraries/System.Private.CoreLib/src/System/Numerics/INumberBase.cs
Outdated
Show resolved
Hide resolved
…erBase.cs Co-authored-by: Miha Zupan <[email protected]>
{ | ||
if (utf16DestinationArray != null) | ||
{ | ||
// Return rented buffers if necessary |
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, I don't think these comments add much
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.
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
src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
Co-authored-by: Dan Moseley <[email protected]>
This change causes C3611 error in my c++/cli project, any idea?
I can simply reproduce it with following sample:
|
Seems like a C++/CLI bug that is exposed when overriding an instance DIM in a derived interface. This doesn't occur if the DIM is done on the interface that defines the API (that is, this wouldn't be an issue if the DIM was done on |
Since
IUtf8SpanFormattable
is a net new interface in .NET 8, then we need to ensure thatINumberBase
implements it in the same release we can't hit a diamond problem in the future.