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.

Sign up for GitHub

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

Enable funlen linter (part 2) #1171

Merged
merged 13 commits into from
Apr 24, 2024
Merged

Enable funlen linter (part 2) #1171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0b8daa4
refactor funlen linter
Mar 27, 2024
ed24a6e
added missing tests for the refactored functions
Mar 28, 2024
bacf99b
refactor after PR review
Mar 29, 2024
63a94fd
refactor after review
Apr 10, 2024
afdd3e8
fixes after Fuzzing test
Apr 11, 2024
922b301
added test for the specific scenario from the PR
Apr 11, 2024
07d827e
an attempt to resolve merge conflict
Apr 11, 2024
8a1e868
refactor after PR review
Apr 16, 2024
cc8c811
attempt to fix merge conflict
Apr 17, 2024
757f14f
attempt to resolve merge conflict
Apr 17, 2024
0134ea4
merging with main
Apr 17, 2024
a056f81
Merge branch 'master' into master
rekby Apr 23, 2024
013a60c
fix coverage
rekby Apr 23, 2024
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
1 change: 1 addition & 0 deletions internal/allocator/allocator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func New() (v *Allocator) {
return allocatorPool.Get()
}

//nolint:funlen
func (a *Allocator) Free() {
a.valueAllocator.free()
a.typeAllocator.free()
Expand Down
168 changes: 109 additions & 59 deletions internal/decimal/decimal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package decimal
import (
"math/big"
"math/bits"

"github.com/ydb-platform/ydb-go-sdk/v3/internal/xstring"
)

const (
Expand Down Expand Up @@ -99,27 +97,56 @@ func Parse(s string, precision, scale uint32) (*big.Int, error) {
return v, nil
Copy link
Member

Choose a reason for hiding this comment

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

New version has difference with old version of Parse in some cases.

please add fuzzing test for check equals of old and new Parse function results:

func FuzzParse(f *testing.F) {
	f.Fuzz(func(t *testing.T, s string, precision, scale uint32) {
		expectedRes, expectedErr := oldParse(s, precision, scale)
		res, err := Parse(s, precision, scale)
		if expectedErr == nil {
			require.Equal(t, expectedRes, res)
		} else {
			require.Error(t, err)
		}
	})
}

Where oldParse is copy of Parse from main branch.
Now result has differences at arguments "100", 0, 0.

You can past oldParse near the test in decimal_test.go, I will remove it from the branch after merge.

You can add better fuzz-test for better test long numbers instead of random strings only.

You can run fuzzing by command:

go test -fuzz=FuzzParse

https://go.dev/doc/security/fuzz/

Copy link
Author

Choose a reason for hiding this comment

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

added the provided test function FuzzParse and oldParse function to decimal_test.go

}

s, neg, specialValue := setSpecialValue(s, v)
if specialValue != nil {
return specialValue, nil
}
var err error
v, err = parseNumber(s, v, precision, scale, neg)
if err != nil {
return nil, err
}

return v, nil
}

func setSpecialValue(s string, v *big.Int) (string, bool, *big.Int) {
s, neg := parseSign(s)

return parseSpecialValue(s, neg, v)
}

func parseSign(s string) (string, bool) {
neg := s[0] == '-'
Copy link
Member

Choose a reason for hiding this comment

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

it will be good to use function parseSign here

Copy link
Author

Choose a reason for hiding this comment

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

Used existing function parseSign

Copy link
Member

Choose a reason for hiding this comment

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

Now the function setSpecialValue do many things, instead only parse special values.

Now it:

  1. Detect negative values
  2. Cut sign from string

And it has interface with difficult to understand: it change input "v" and return it.
Usually function return new value without change input, rarer change input without return it.

Change and return used for chaining calls within builder - for you can call Obj.Method1().Method2().Method3().... with one line.

please return separate function parseSign function for detect and cut off sign and reuse the function here and in parseNumber.

Or detect and cut sign before call setSpecialValue (better parseSpecialValue) and pass the sign to the function, then to parseNumber if you won't check the sign twice.

P.S. i think about check the sign twice better - it is very small check, but it more understandable to read, then many parameters.

if neg || s[0] == '+' {
s = s[1:]
}

return s, neg
}

func parseSpecialValue(s string, neg bool, v *big.Int) (string, bool, *big.Int) {
if isInf(s) {
if neg {
rekby marked this conversation as resolved.
Show resolved Hide resolved
return v.Set(neginf), nil
return s, neg, v.Set(neginf)
}

return v.Set(inf), nil
return s, neg, v.Set(inf)
}
if isNaN(s) {
if neg {
return v.Set(negnan), nil
return s, neg, v.Set(negnan)
}

return v.Set(nan), nil
return s, neg, v.Set(nan)
}

integral := precision - scale
return s, neg, nil
}

func parseNumber(s string, v *big.Int, precision, scale uint32, neg bool) (*big.Int, error) {
var err error
integral := precision - scale
var dot bool
for ; len(s) > 0; s = s[1:] {
c := s[0]
Expand All @@ -131,12 +158,10 @@ func Parse(s string, precision, scale uint32) (*big.Int, error) {

continue
}
if dot {
if scale > 0 {
scale--
} else {
break
}
if dot && scale > 0 {
scale--
} else if dot {
break
}

if !isDigit(c) {
Expand All @@ -155,30 +180,10 @@ func Parse(s string, precision, scale uint32) (*big.Int, error) {
}
integral--
}
//nolint:nestif
if len(s) > 0 { // Characters remaining.
c := s[0]
if !isDigit(c) {
return nil, syntaxError(s)
}
plus := c > '5'
if !plus && c == '5' {
var x big.Int
plus = x.And(v, one).Cmp(zero) != 0 // Last digit is not a zero.
for !plus && len(s) > 1 {
s = s[1:]
c := s[0]
if !isDigit(c) {
return nil, syntaxError(s)
}
plus = c != '0'
}
}
if plus {
v.Add(v, one)
if v.Cmp(pow(ten, precision)) >= 0 {
v.Set(inf)
}
v, err = handleRemainingDigits(s, v, precision)
if err != nil {
return nil, err
}
}
v.Mul(v, pow(ten, scale))
Expand All @@ -189,26 +194,54 @@ func Parse(s string, precision, scale uint32) (*big.Int, error) {
return v, nil
}

func handleRemainingDigits(s string, v *big.Int, precision uint32) (*big.Int, error) {
c := s[0]
if !isDigit(c) {
return nil, syntaxError(s)
}
plus := c > '5'
if !plus && c == '5' {
var x big.Int
plus = x.And(v, one).Cmp(zero) != 0 // Last digit is not a zero.
for !plus && len(s) > 1 {
s = s[1:]
c := s[0]
if !isDigit(c) {
return nil, syntaxError(s)
}
plus = c != '0'
}
}
if plus {
v.Add(v, one)
if v.Cmp(pow(ten, precision)) >= 0 {
v.Set(inf)
}
}

return v, nil
}

// Format returns the string representation of x with the given precision and
// scale.
func Format(x *big.Int, precision, scale uint32) string {
switch {
case x.CmpAbs(inf) == 0:
// Check for special values and nil pointer upfront.
if x == nil {
return "0"
}
if x.CmpAbs(inf) == 0 {
if x.Sign() < 0 {
return "-inf"
}

return "inf"

case x.CmpAbs(nan) == 0:
}
if x.CmpAbs(nan) == 0 {
if x.Sign() < 0 {
return "-nan"
}

return "nan"

case x == nil:
return "0"
}

v := big.NewInt(0).Set(x)
Expand All @@ -232,42 +265,59 @@ func Format(x *big.Int, precision, scale uint32) string {

digit.Mod(v, ten)
d := int(digit.Int64())
if d != 0 || scale == 0 || pos > 0 {
const numbers = "0123456789"
pos--
bts[pos] = numbers[d]

pos--
if d != 0 || scale == 0 || pos >= 0 {
setDigitAtPosition(bts, pos, d)
}

if scale > 0 {
scale--
if scale == 0 && pos > 0 {
bts[pos-1] = '.'
pos--
bts[pos] = '.'
}
}
}
if scale > 0 {
for ; scale > 0; scale-- {
if precision == 0 {
return errorTag
}
precision--
pos--
bts[pos] = '0'
}

for ; scale > 0; scale-- {
if precision == 0 {
pos = 0

break
}
precision--
pos--
bts[pos] = '.'
bts[pos] = '0'
}

if bts[pos] == '.' {
pos--
bts[pos] = '0'
}

if neg {
pos--
bts[pos] = '-'
}

return xstring.FromBytes(bts[pos:])
return string(bts[pos:])
}

func abs(x *big.Int) (*big.Int, bool) {
v := big.NewInt(0).Set(x)
neg := x.Sign() < 0
if neg {
// Convert negative to positive.
v.Neg(x)
}

return v, neg
}

func setDigitAtPosition(bts []byte, pos, digit int) {
const numbers = "0123456789"
bts[pos] = numbers[digit]
}

// BigIntToByte returns the 16-byte array representation of x.
Expand Down
Loading
Loading