Prototyp, BigInt string parsing, added functions for parsing from string, fix panic #153
Conversation
david-littlefarmer
force-pushed
the
prototyp_base_panic_fix
branch
from
955f7c5 to
894189f
Compare
| if (char >= 48 && char <= 57) || char == 45 || char == 66 || char == 98 { | ||
| ns.WriteRune(char) |
There was a problem hiding this comment.
btw: if we see a character that doesn't fit into this condition... shouldn't we error out instead of ignoring it?
There was a problem hiding this comment.
We should, but it is written in a way, that it ignores other characters.
So 1a2b3c4d is parsed to 1234
I can't change it now, it would be breaking change :/
There was a problem hiding this comment.
I think it would be for better. Ignoring some characters and treating the input as valid sounds like a bug to me.
Also, I think we should introduce a function that returns an error.
func ParseNumberString() (int64, error) {}but I guess we need to keep the existing function backward-compatible, so we'd need to come up with a different func name
@pkieltyka thoughts?
There was a problem hiding this comment.
we definitely need backwards compatibility, as well what would we use func ParseNumberString() (int64, error) {} for..? if anything we should have: ParseNumberStringInt64 and ParseNumberStringBigInt for example.. I think we can do those in a separate PR, lets keep this PR focused to get the bug fixed
| {NewBigIntFromHexString, "2A", 42, "Hex 2A"}, | ||
| {NewBigIntFromHexString, "2a", 42, "Hex 2a"}, | ||
| {NewBigIntFromHexString, "-2A", -42, "Hex -2A"}, | ||
| {NewBigIntFromHexString, "-2a", -42, "Hex -2a"}, | ||
| {NewBigIntFromHexString, "0x2a", 42, "Hex with 0x 42"}, | ||
| {NewBigIntFromHexString, "0X2a", 42, "Hex with 0X 42"}, |
There was a problem hiding this comment.
It'd be nice to add some test cases with invalid inputs & test for errors
| @@ -30,11 +43,16 @@ func NewBigIntFromHexString(hs string) BigInt { | |||
|
|
|||
| func NewBigIntFromString(s string, base int) BigInt { | |||
| b := big.NewInt(0) | |||
| if base != 0 && (2 > base || base > big.MaxBase) { | |||
There was a problem hiding this comment.
this condition doesn't read very well
There was a problem hiding this comment.
it is horrible, but it the same conditon which is inside math package and which leads to panic. I can add a comment with link to that code.
| case 2: | ||
| for _, char := range s { | ||
| // 0-9 || B || b | ||
| if (char >= 48 && char <= 57) || char == 45 || char == 66 || char == 98 { |
There was a problem hiding this comment.
Is there a reason char == 45 is acceptable here but not in the other cases? We should update the comment if there's a specific reason.
There was a problem hiding this comment.
good catch, here is fix: #166
Added tests, and added parsing helper functions for binary and octal bases and unified naming. Everything is backward compatible.
The
bigpackage is throwing panic on base out of boundaries: https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/math/big/natconv.go;l=114So when base is out of boundaries, i return 0, instead of panicking.
This:
is panicking
