Fixes of invalid biginteger casting #56710
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThis PR fix #49611.
|
| @@ -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) | |||
There was a problem hiding this comment.
Should it be IsNull instead? Maybe we can inline the bits == null check at the callsite (as there is only one caller).
There was a problem hiding this comment.
Yeap, you are right. I'll fix it after complete review
| @@ -2433,6 +2424,36 @@ public long GetBitLength() | |||
| return bitLength - 1; | |||
| } | |||
|
|
|||
| private static double ConvertToDouble(uint[] bits, | |||
There was a problem hiding this comment.
nit: We don't normally have a parameter per line.
| @@ -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) | |||
| { | |||
| bool returnValue; | |||
There was a problem hiding this comment.
Why isn't this simply return bits is null?
| { | ||
| bool returnValue; | ||
|
|
||
| // The maximum exponent for doubles is 1023, which corresponds to a uint bit length of 32. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
The last sentence also looks to be no longer true.
There was a problem hiding this comment.
@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.
| private static double ConvertToDouble(uint[] bits, | ||
| int length, | ||
| int sign) | ||
| { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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?
|
@Maximys are you still working on this? No particular rush, just wanted to ensure that was the case since many people took a break over the holidays. There are still several pending comments above that would need to be resolved. |
tannergooding
added
needs-author-action
|
@tannergooding , I forgot about this PR. I'll try to process pending comments today. |
ghost
removed
the
needs-author-action
tannergooding
added
the
needs-author-action
|
This pull request has been automatically marked |
|
This pull request will now be closed since it had been marked |
ghost
closed this
ghost
locked as resolved and limited conversation to collaborators
This PR fix #49611.