Skip to content

Add Decimal32, Decimal64, Decimal128#100729

Open
RaymondHuy wants to merge 97 commits intodotnet:mainfrom
RaymondHuy:issue-81376
Open

Add Decimal32, Decimal64, Decimal128#100729
RaymondHuy wants to merge 97 commits intodotnet:mainfrom
RaymondHuy:issue-81376

Conversation

@RaymondHuy
Copy link
Copy Markdown
Contributor

Resolve #81376

@ghost
Copy link
Copy Markdown

ghost commented Apr 6, 2024

Note regarding the new-api-needs-documentation label:

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.

@RaymondHuy RaymondHuy marked this pull request as draft April 6, 2024 17:29
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Apr 6, 2024
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs Outdated
static int IDecimalIeee754UnpackInfo<Decimal128, Int128, UInt128>.NumberDigitsPrecision => NumberDigitsPrecision;


private static Int128[] Int128Powers10 =>
Copy link
Copy Markdown
Member

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

Comment on lines +104 to +107
public override int GetHashCode()
{
return _value.GetHashCode();
}
Copy link
Copy Markdown
Member

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.

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;
}

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
@tannergooding
Copy link
Copy Markdown
Member

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

@RaymondHuy RaymondHuy marked this pull request as ready for review April 9, 2024 17:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal32.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs Outdated
@tannergooding
Copy link
Copy Markdown
Member

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.

Copilot AI review requested due to automatic review settings May 3, 2026 15:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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) and public 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).

Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal32.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
Comment thread src/libraries/Common/src/System/Number.NumberBuffer.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Decimal64Tests.cs Outdated
Copilot AI review requested due to automatic review settings May 5, 2026 17:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
Copilot AI review requested due to automatic review settings May 5, 2026 18:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs
Copilot AI review requested due to automatic review settings May 6, 2026 14:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 implement IFormattable. 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 parameterless ToString(). Consider adding System.IFormattable to Decimal32/Decimal64/Decimal128 (and updating the CoreLib implementations accordingly); optionally also implement ISpanFormattable to match other numerics.

Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Decimal64Tests.cs Outdated
@RaymondHuy
Copy link
Copy Markdown
Contributor Author

Hi @tannergooding I have resolved all Copilot comments. You can re-review it when you have time ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Numerics community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add Decimal32, Decimal64, and Decimal128 from the IEEE 754-2019 standard.

9 participants