Restrict the maximum length of BigInteger #102874
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
|
Some overflow checks in ToString and DebuggerDisplay can also be simplified. |
Yep, but I plan on handling those in a separate PR. This is just the simplest change to give consistent behavior with the existing logiic. I'm working (slowly) on a change to switch to using |
tannergooding
force-pushed
the
bigint-maxlength
branch
2 times, most recently
from
b4f9f7b
to
de72853
Compare
tannergooding
force-pushed
the
bigint-maxlength
branch
3 times, most recently
from
dc91832
to
8bf392e
Compare
tannergooding
force-pushed
the
bigint-maxlength
branch
3 times, most recently
from
2677779
to
ee08234
Compare
tannergooding
force-pushed
the
bigint-maxlength
branch
from
ee08234
to
8a1ebd8
Compare
| @@ -527,87 +536,88 @@ private BigInteger(ReadOnlySpan<uint> value, bool negative) | |||
| /// <param name="value"></param> | |||
| private BigInteger(Span<uint> value) | |||
There was a problem hiding this comment.
I'm curious why we have two separate implementations one for Span<uint> and one for ReadOnlySpan<uint> + isNegative. Could this ctor just do the checks to compute isNegative and then delegate to the other in order to avoid duplication? Or are they dealing with completely different formats?
There was a problem hiding this comment.
Mostly because BigInteger is an old type in desperate need of a rewrite 😆. I imagine we could, with a bit more refactoring, share some or most of the implementation here.
private BigInteger(ReadOnlySpan<uint> value, bool negative) is an optimized implementation that assumes we are already in almost the right shape. So value is the unsigned storage data (the absolute value) and we're really just trimming unnecessary leading zeros and deciding whether to store it in _bits or compress it into _sign.
private BigInteger(Span<uint> value) on the other hand is taking a two's complement little-endian span. So it has to detect if its positive or negative and if its negative fix it up to be the absolute value for storage purposes. So I expect we could implement it as:
- determine if negative, trimming leading sign data (
0xFFif negative,0x00if positive) - if negative, compute the two's complement (absolute value)
- call
private BigInteger(ReadOnlySpan<uint> value, bool negative)
| @@ -29,19 +29,21 @@ public static void BigShiftsTest() | |||
| public static void LargeNegativeBigIntegerShiftTest() | |||
| { | |||
| // Create a very large negative BigInteger | |||
| BigInteger bigInt = new BigInteger(-1) << int.MaxValue; | |||
| Assert.Equal(2147483647, bigInt.GetBitLength()); | |||
There was a problem hiding this comment.
Should this become a test that validates an exception is thrown?
There was a problem hiding this comment.
Potentially. There's some general cleanup and additional validation I'd like to do here long term, but I'll get this back up to validate it throws in a follow up PR.
As per the comment in the code we have several APIs that are already restricted and will throw for big integers above this limit, so this just enforces it consistently. An almost 256MB BigInteger is enormous and well above any typical input range, it is more than enough for the in-box type to support and helps avoid other edge case considerations for users. It will also allow us to bound the computation time for some APIs in a more sensible fashion.