Skip to content
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

Fixes of invalid biginteger casting #56710

Original file line number Diff line number Diff line change
Expand Up @@ -1799,35 +1799,26 @@ public static explicit operator double(BigInteger value)

int sign = value._sign;
uint[]? bits = value._bits;
double returnValue;

if (bits == null)
return sign;

int length = bits.Length;

// The maximum exponent for doubles is 1023, which corresponds to a uint bit length of 32.
// All BigIntegers with bits[] longer than 32 evaluate to Double.Infinity (or NegativeInfinity).
// Cases where the exponent is between 1024 and 1035 are handled in NumericsHelpers.GetDoubleFromParts.
const int InfinityLength = 1024 / kcbitUint;

if (length > InfinityLength)
if (IsEmpty(bits))
{
if (sign == 1)
return double.PositiveInfinity;
returnValue = sign;
}
else
{
int length = bits.Length;
if (!TryGetInfinity(length, sign, out double? rawValue))
{
returnValue = ConvertToDouble(bits, length, sign);
}
else
return double.NegativeInfinity;
{
returnValue = rawValue.Value;
}
}

ulong h = bits[length - 1];
ulong m = length > 1 ? bits[length - 2] : 0;
ulong l = length > 2 ? bits[length - 3] : 0;

int z = NumericsHelpers.CbitHighZero((uint)h);

int exp = (length - 2) * 32 - z;
ulong man = (h << 32 + z) | (m << z) | (l >> 32 - z);

return NumericsHelpers.GetDoubleFromParts(sign, exp, man);
return returnValue;
}

public static explicit operator decimal(BigInteger value)
Expand Down Expand Up @@ -2433,6 +2424,36 @@ public long GetBitLength()
return bitLength - 1;
}

private static double ConvertToDouble(uint[] bits,
Copy link
Member

Choose a reason for hiding this comment

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

nit: We don't normally have a parameter per line.

int length,
int sign)
{
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to try and normalize this to be the same as the already known "good" logic used in our double parsing/formatting logic:
https://source.dot.net/#System.Private.CoreLib/Number.Parsing.cs,9927df209c6039f5,references and https://source.dot.net/#System.Private.CoreLib/Number.NumberToFloatingPointBits.cs,3b99f12cbc14a09d

The overall logic is a bit more complex since it needs to handle non integral numbers; but it also correctly handles rounding and various edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding, thatk you for your comment, I had not know about it. I agree, that a widely used method is better than a new one, but I also believe that the provided methods contain more logic than necessary and may work little slower.
I'll try to understand provided methods and apply some one for remove my 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.

@tannergooding , I can't understand how to use https://source.dot.net/#System.Private.CoreLib/Number.Parsing.cs,9927df209c6039f5,references , because I can not add reference to System.Private.CoreLib .
Can you provide little example about reusing Number.NumberToDouble?

ulong highBit, middleBit;
ulong nonZeroDigit;
int leftShiftingSize;
double unsignedNumber, returnValue;

const int NonZeroDigitsCountOfUInt32 = 32;

highBit = bits[length - 1];
middleBit = length > 1 ? bits[length - 2] : 0;

leftShiftingSize = (length - 2) * NonZeroDigitsCountOfUInt32;
nonZeroDigit = (highBit << NonZeroDigitsCountOfUInt32) | middleBit;
unsignedNumber = Math.ScaleB(nonZeroDigit, leftShiftingSize);

if (sign < 0)
{
returnValue = -unsignedNumber;
}
else
{
returnValue = unsignedNumber;
}

return returnValue;
}

/// <summary>
/// Encapsulate the logic of normalizing the "small" and "large" forms of BigInteger
/// into the "large" form so that Bit Manipulation algorithms can be simplified.
Expand Down Expand Up @@ -2461,6 +2482,54 @@ private static bool GetPartsForBitManipulation(ref BigInteger x, ref Span<uint>
return x._sign < 0;
}

private static bool IsEmpty([NotNullWhen(false)] uint[]? bits)
Copy link
Member

Choose a reason for hiding this comment

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

Should it be IsNull instead? Maybe we can inline the bits == null check at the callsite (as there is only one caller).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, you are right. I'll fix it after complete review

{
bool returnValue;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this simply return bits is null?


if (bits == null)
{
returnValue = true;
}
else
{
returnValue = false;
}

return returnValue;
}

private static bool TryGetInfinity(int length,
int sign,
[NotNullWhen(true)] out double? infinityValue)
{
bool returnValue;

// The maximum exponent for doubles is 1023, which corresponds to a uint bit length of 32.
Copy link
Member

Choose a reason for hiding this comment

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

nit: The comment of bit length of 32 isn't great (I know it was already there before this PR) as reading it sounds like # of bits is more than 32 (implying a 32-bit integer).

Copy link
Member

Choose a reason for hiding this comment

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

The last sentence also looks to be no longer true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding , I think, comment is evil and Uncle Bob ( http://cleancoder.com/products ) absolutely agree with me. I had wanted to remove this comment, but it can very hard to understand what's the magic value 1024 in line below. I'll try to change this comment to more clear.

// All BigIntegers with bits[] longer than 32 evaluate to Double.Infinity (or NegativeInfinity).
// Cases where the exponent is between 1024 and 1035 are handled in NumericsHelpers.GetDoubleFromParts.
const int InfinityLength = 1024 / kcbitUint;

if (length > InfinityLength)
{
if (sign == 1)
{
infinityValue = double.PositiveInfinity;
}
else
{
infinityValue = double.NegativeInfinity;
}
returnValue = true;
}
else
{
infinityValue = null;
returnValue = false;
}

return returnValue;
}

internal static int GetDiffLength(uint[] rgu1, uint[] rgu2, int cu)
{
for (int iv = cu; --iv >= 0;)
Expand Down
12 changes: 12 additions & 0 deletions src/libraries/System.Runtime.Numerics/tests/BigInteger/MyBigInt.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ namespace System.Numerics.Tests
{
public static class MyBigIntImp
{
private const byte NegativeResetter = 0b0111_1111;

public static BigInteger DoUnaryOperatorMine(BigInteger num1, string op)
{
List<byte> bytes1 = new List<byte>(num1.ToByteArray());
Expand Down Expand Up @@ -943,6 +945,16 @@ public static byte[] GetRandomByteArray(Random random, int size)
return value;
}

public static byte[] GetRandomPosByteArray(Random random, int size)
{
byte[] returnValue;

returnValue = GetRandomByteArray(random, size);
returnValue[returnValue.Length - 1] &= NegativeResetter;

return returnValue;
}

public static BigInteger ApproximateBigInteger(double value)
{
//Special case values;
Expand Down
Loading