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

Prototyp, BigInt string parsing, added functions for parsing from string, fix panic #153

Merged
merged 1 commit into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 51 additions & 4 deletions lib/prototyp/bigint.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,20 @@ func NewBigInt(n int64) BigInt {
return BigInt(*b)
}

// Decimal number string
func NewBigIntFromNumberString(hs string) BigInt {
return NewBigIntFromDecimalString(hs)
}

func NewBigIntFromBinaryString(hs string) BigInt {
return NewBigIntFromString(hs, 2)
}

func NewBigIntFromOctalString(hs string) BigInt {
return NewBigIntFromString(hs, 8)
}

func NewBigIntFromDecimalString(hs string) BigInt {
return NewBigIntFromString(hs, 10)
}

Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this condition doesn't read very well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return BigInt{}
}

b, ok := b.SetString(s, base)
if !ok {
n, _ := ParseNumberString(s, base)
b = big.NewInt(n)
}

return BigInt(*b)
}

Expand Down Expand Up @@ -228,29 +246,58 @@ func (b *BigInt) UnmarshalBinary(buff []byte) error {

func ParseNumberString(s string, base int) (int64, bool) {
var ns strings.Builder
if base == 10 {
switch base {
case 2:
for _, char := range s {
// 0-9 || B || b
if (char >= 48 && char <= 57) || char == 45 || char == 66 || char == 98 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, here is fix: #166

ns.WriteRune(char)
Comment on lines +253 to +254
Copy link
Contributor

Choose a reason for hiding this comment

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

btw: if we see a character that doesn't fit into this condition... shouldn't we error out instead of ignoring it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :/

Copy link
Contributor

@VojtechVitek VojtechVitek Sep 25, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
s = ns.String()
s = strings.TrimPrefix(s, "0B")
s = strings.TrimPrefix(s, "0b")

case 8:
for _, char := range s {
// 0-9 || O || o
if (char >= 48 && char <= 57) || char == 79 || char == 111 {
ns.WriteRune(char)
}
}
s = ns.String()
s = strings.TrimPrefix(s, "0O")
s = strings.TrimPrefix(s, "0o")

case 10:
for _, char := range s {
// 0-9
if char >= 48 && char <= 57 {
ns.WriteRune(char)
}
}
s = ns.String()
} else if base == 16 {

case 16:
for _, char := range s {
if (char >= 48 && char <= 57) || (char == 120) || (char >= 65 && char <= 70) || (char >= 97 && char <= 102) {
// 0-9 || A-Z || a-z || X || x
if (char >= 48 && char <= 57) || (char >= 65 && char <= 70) || (char >= 97 && char <= 102) || char == 88 || char == 120 {
ns.WriteRune(char)
}
}

s = ns.String()
s = strings.TrimPrefix(s, "0X")
s = strings.TrimPrefix(s, "0x")
} else {
default:
return 0, false
}

n, err := strconv.ParseInt(s, base, 64)
if err != nil {
return 0, false
}

return n, true
}

Expand Down
50 changes: 47 additions & 3 deletions lib/prototyp/bigint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package prototyp

import (
"encoding/json"
"fmt"
"math/big"
"testing"

Expand Down Expand Up @@ -58,6 +57,46 @@ func TestBigIntScan(t *testing.T) {
assert.Error(t, b.Scan("1."))
}

func TestBigIntFromBaseString(t *testing.T) {
testCases := []struct {
fn func(string) BigInt
input string
expected int64
testName string
}{
// Binary tests
{NewBigIntFromBinaryString, "00101010", 42, "Binary 42"},
{NewBigIntFromBinaryString, "-00101010", -42, "Binary -42"},
{NewBigIntFromBinaryString, "0B00101010", 42, "Binary with 0B 42"},
{NewBigIntFromBinaryString, "0b00101010", 42, "Binary with 0b 42"},

// Octal tests
{NewBigIntFromOctalString, "52", 42, "Octal 42"},
{NewBigIntFromOctalString, "-52", -42, "Octal -42"},
{NewBigIntFromOctalString, "0O52", 42, "Octal with 0O 42"},
{NewBigIntFromOctalString, "0o52", 42, "Octal with 0o 42"},

// Decimal tests
{NewBigIntFromDecimalString, "42", 42, "Decimal 42"},
{NewBigIntFromDecimalString, "-42", -42, "Decimal -42"},

// Hexadecimal tests
{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"},
Comment on lines +84 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to add some test cases with invalid inputs & test for errors

}

for _, tc := range testCases {
t.Run(tc.testName, func(t *testing.T) {
b := tc.fn(tc.input)
assert.Equal(t, tc.expected, b.Int64())
})
}
}

func TestBigIntInvalidInput(t *testing.T) {
b := NewBigIntFromNumberString("1234")
assert.Equal(t, int64(1234), b.Int64())
Expand All @@ -69,12 +108,17 @@ func TestBigIntInvalidInput(t *testing.T) {
// if invalid, it will parse out the number
b = NewBigIntFromHexString("/0xaBc-$$2Efg")
assert.Equal(t, int64(0xaBc2Ef), b.Int64())

// should return 0, since base is not valid
b = NewBigIntFromString("1234", 666)
assert.Equal(t, int64(0), b.Int64())
}

func TestBigIntCopy(t *testing.T) {
a := ToBigInt(big.NewInt(1))
b := ToBigInt(a.Int())
a.Sub(big.NewInt(1))
fmt.Println(a.String())
fmt.Println(b.String())

assert.Equal(t, int64(0), a.Int64())
assert.Equal(t, int64(1), b.Int64())
}
Loading