Updating System.Runtime.InteropServices.NFloat to support generic math #68141
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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. |
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/interop-contrib Issue Detailsnull
|
| { | ||
| NativeType tmp = value._value; | ||
| ++tmp; | ||
| return new NFloat(tmp); |
There was a problem hiding this comment.
Did this change because + 1 and ++ have different semantics? Or just out of style consistency of ensuring the implementation of the operator uses the same operator?
There was a problem hiding this comment.
Consistency with how the other types which needed to do this where doing this.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NFloat.cs
Show resolved
Hide resolved
| ThrowHelper.ThrowNotSupportedException(); | ||
| result = default; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I don't see anything in CreateChecked that throws in places this doesn't (maybe I missed it?). Can this implementation just delegate to that, or vice versa?
There was a problem hiding this comment.
Probably can in this case. The integer types can't do this since they actually have overflow behavior, but the floating-point types can since they don't.
There was a problem hiding this comment.
Will update all 4 floating-point types to delegate in a follow up PR to avoid spinning CI again.
Right. Generally not testing the function in depth, but rather the bare minimum its bound/working correctly when called via The proper testing already exists for these members on the relevant type |
|
Merged with dotnet/main to resolve merge conflict |
|
Merged with main and fixed up the implementation to include the new APIs required by #68094 |
dotnet#68141) * Updating NFloat to implement IBinaryFloatingPointIeee754 and IMinMaxValue * Adding tests for the NFloat generic math support * Fixing NFloat to expose the APIs for getting the exponent and significand * Fix the NFloat.GetSignificandByteCount implementation
|
This causes build bot failures on s390x, due to what looks like endian bugs: |
| if (destination.Length >= sizeof(NativeExponentType)) | ||
| { | ||
| NativeExponentType exponent = _value.Exponent; | ||
| Unsafe.WriteUnaligned(ref MemoryMarshal.GetReference(destination), exponent); |
There was a problem hiding this comment.
This write isn't endian-aware when it probably should be.
There was a problem hiding this comment.
Will get up a PR handling this.
| if (destination.Length >= sizeof(NativeSignificandType)) | ||
| { | ||
| NativeSignificandType significand = _value.Significand; | ||
| Unsafe.WriteUnaligned(ref MemoryMarshal.GetReference(destination), significand); |
No description provided.