-
Notifications
You must be signed in to change notification settings - Fork 5k
Add Decimal32, Decimal64, Decimal128 #100729
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
base: main
Are you sure you want to change the base?
Conversation
Note regarding the
|
src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs
Outdated
Show resolved
Hide resolved
static int IDecimalIeee754UnpackInfo<Decimal128, Int128, UInt128>.NumberDigitsPrecision => NumberDigitsPrecision; | ||
|
||
|
||
private static Int128[] Int128Powers10 => |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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.
runtime/src/libraries/System.Private.CoreLib/src/System/Numerics/TotalOrderIeee754Comparer.cs
Lines 125 to 132 in 9b57a26
else | |
{ | |
// Equivalant values are compared by their exponent parts, | |
// and the value with smaller exponent is considered closer to zero. | |
// This only applies to IEEE 754 decimals. Consider to add support if decimals are added into .NET. | |
return 0; | |
} |
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.
@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.
src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs
Outdated
Show resolved
Hide resolved
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 |
This reverts commit 79bea65.
Hi @tannergooding I have resolved almost your comments (some places need your resolve confirmation). For the constructor API case, I don't know what I should do with it because I think you are going to raise it in the API Proposal to remove this constructor. |
Hi @tannergooding this PR is ready to review, I have separated the processing flow for |
Resolve #81376