Add Decimal32, Decimal64, Decimal128#100729
Conversation
|
Note regarding the |
| static int IDecimalIeee754UnpackInfo<Decimal128, Int128, UInt128>.NumberDigitsPrecision => NumberDigitsPrecision; | ||
|
|
||
|
|
||
| private static Int128[] Int128Powers10 => |
There was a problem hiding this comment.
If Int128 is not optimized, this can be optimally stored as a ReadOnlySpan of Int32/Int64 like Number.BigInteger.Pow10BigNumTable
| public override int GetHashCode() | ||
| { | ||
| return _value.GetHashCode(); | ||
| } |
There was a problem hiding this comment.
+0 and -0 should have the same hash code. The easiest way can be stripping the sign bit.
I also remember that there can be same values with different representations.
There was a problem hiding this comment.
@huoyaoyuan Thanks for your review. However this PR targets only the first phase as following: #81376 (comment) so I haven't added NegativeZero and PositiveZero. I will update GetHashCode method in the next phase.
|
This is on my radar to take a look at; just noting it might be a bit delayed due to other priorities for the .NET 9 release. CC. @jeffhandley |
|
A lot of the copilot feedback looks correct/actionable. I'm working on another review pass to make sure all the edge cases were handled. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (2)
src/libraries/System.Runtime/ref/System.Runtime.cs:1
- The reference assembly declarations for Decimal32/Decimal64/Decimal128 appear to be missing public members that exist in the implementations (notably
public override bool Equals(object? obj)andpublic override int GetHashCode()). Ref/impl mismatches will break API surface validation and can cause binding inconsistencies. Add these overrides (and any other public members present in the CoreLib implementations) to the ref file so the public contract matches.
src/libraries/System.Runtime/ref/System.Runtime.cs:1 - The Decimal32 implementation is annotated with
[StructLayout(LayoutKind.Sequential)], but the ref assembly declaration does not include this attribute. Since layout is part of the public interop contract, the ref should mirror it (or the implementation should drop it if not required).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime/ref/System.Runtime.cs:1
- These new numeric types expose
ToString(string?, IFormatProvider?)overloads but do not implementIFormattable. As a result, composite formatting / interpolated string format specifiers (e.g.,string.Format(\"{0:N}\", value)or$\"{value:N}\") may ignore the format string and fall back to parameterlessToString(). Consider addingSystem.IFormattabletoDecimal32/Decimal64/Decimal128(and updating the CoreLib implementations accordingly); optionally also implementISpanFormattableto match other numerics.
|
Hi @tannergooding I have resolved all Copilot comments. You can re-review it when you have time ;) |
Resolve #81376