Skip to content

Conversation

@RaymondHuy
Copy link
Contributor

Resolve #81376

@ghost
Copy link

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
static int IDecimalIeee754UnpackInfo<Decimal128, Int128, UInt128>.NumberDigitsPrecision => NumberDigitsPrecision;


private static Int128[] Int128Powers10 =>
Copy link
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
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
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.

@tannergooding
Copy link
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
}
}

private static unsafe TValue DecimalIeee754BinaryEncoding<TDecimal, TValue>(bool signed, TValue significand, int exponent)
Copy link
Member

Choose a reason for hiding this comment

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

nit: exponent and significand are swapped compared to the typical order of such APIs

Copy link
Member

Choose a reason for hiding this comment

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

These types of APIs, even though they are private/internal, likely also benefit from XML doc comments to be explicit that that significand is the "full" (non-trailing) significand and exponent is the unbiased exponent.

The terms get interchanged a lot and so being explicit can help ensure future readers understand the intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have updated the method name and added comments to help it readable.

byte[] buffer = ArrayPool<byte>.Shared.Rent(TDecimal.BufferLength);
try
{
fixed (byte* pDigits = buffer)
Copy link
Member

Choose a reason for hiding this comment

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

more cases that are using unnecessary unsafe. NumberBuffer takes a span

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have updated it.

Comment on lines 347 to 348
char* stackPtr = stackalloc char[CharStackBufferSize];
var vlb = new ValueListBuilder<char>(new Span<char>(stackPtr, CharStackBufferSize));
Copy link
Member

Choose a reason for hiding this comment

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

this can directly use stackalloc in the ValueListBuilder constructor as well, since the language will convert it to span implicitly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tannergooding , I follow your instruction, but it throws this error,
image
do you have another suggestion, or we just keep the old one.

Copy link
Member

@tannergooding tannergooding Nov 18, 2025

Choose a reason for hiding this comment

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

Yeah, unfortunately there's actually a missing language feature here due to how we pass the data to NumberToString. We need ref scoped SomeRefStruct to do this one in particular.

Other callouts may not need that feature, just this one in particular does.

Comment on lines 364 to 367
finally
{
ArrayPool<byte>.Shared.Return(buffer);
}
Copy link
Member

Choose a reason for hiding this comment

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

We don't typically put the return in a finally block. An exception being thrown is very unexpected and returning is not required for correctness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have removed the finally block.

where TValue : unmanaged, IBinaryInteger<TValue>
{
byte* buffer = number.DigitsPtr;
DecimalIeee754<TValue> unpackDecimal = value.Unpack();
Copy link
Member

Choose a reason for hiding this comment

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

Unpack should likely not be an extension method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tannergooding , it is not an extension method, it is a method declared in IDecimalIeee754ParseAndFormatInfo interface

DecimalIeee754<TValue> unpackDecimal = value.Unpack();
number.IsNegative = unpackDecimal.Signed;

byte* p = buffer + TDecimal.Precision;
Copy link
Member

Choose a reason for hiding this comment

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

More cases that should use Span instead. There's a number of them and I don't plan to comment every instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have updated it.

Comment on lines 33 to 36
private static readonly UInt128 PositiveInfinityValue = new UInt128(upper: 0x7800_0000_0000_0000, lower: 0);
private static readonly UInt128 NegativeInfinityValue = new UInt128(upper: 0xf800_0000_0000_0000, lower: 0);
private static readonly UInt128 ZeroValue = new UInt128(0, 0);
private static readonly UInt128 NegativeZeroValue = new UInt128(0x8000_0000_0000_0000, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a problem for codegen since UInt128 is not const?

Copy link
Member

Choose a reason for hiding this comment

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

They should probably be properties instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have updated it.

return Number.FormatDecimalIeee754<Decimal128, UInt128>(this, format, NumberFormatInfo.GetInstance(provider));
}

private static UInt128[] UInt128Powers10 =>
Copy link
Member

Choose a reason for hiding this comment

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

This can be a problem for constant embedding. long array can be encoded into executable and read by pairs.

@znakeeye
Copy link

Are there any preliminary benchmarks of these new types? I.e. How do decimal128 and decimal64 compare to the existing decimal type?

Also, can we expect these new types to arrive in .NET 11?

@huoyaoyuan
Copy link
Member

Are there any preliminary benchmarks of these new types? I.e. How do decimal128 and decimal64 compare to the existing decimal type?

We are focusing on the different semantic first. Both types are having different existing standard to follow.

Also, can we expect these new types to arrive in .NET 11?

It's optimistic, but not a guarantee.

@RaymondHuy
Copy link
Contributor Author

Hi @tannergooding, thanks for your review, I have resolved almost your comments (handle NaN, TDecimal.MaxValue + (Delta / 2) vs TDecimal.MaxValue + ((Delta / 2) - 1) rounding, remove unsafe, update parameter names, added comments). Can you help me review it again ?

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.

7 participants