decimal icon indicating copy to clipboard operation
decimal copied to clipboard

Invalid Cmp result after using SetString multiple times on same Big

Open comunidadio opened this issue 2 years ago • 2 comments

Experienced very weird bug with routine that does reuse to avoid allocations. It appears reusing by SetFloat worked fine all along, but reusing by SetString breaks.

Managed to assemble minimal testcase below.

TestCmpWorksWithSetFloat64 passes. The same logic in TestCmpWorksWithSetString breaks with:

Error:           Not equal:       
                     expected: -1
                     actual  : 1
Test:             TestCmpBreaksWithSetString
Messages:   1571.5 < 1.57

Minimal testcase:

package main

import (
	"fmt"
	"testing"

	"github.com/ericlagergren/decimal"
	"github.com/stretchr/testify/assert"
)

func TestCmpWorksWithSetFloat64(t *testing.T) {
	x := &decimal.Big{}
	y := &decimal.Big{}
	x.SetFloat64(1.57)
	y.Set(x)
	x.SetFloat64(1.5)
	assert.Equal(t, -1, x.Cmp(y), fmt.Sprintf("%s < %s", x.String(), y.String()))
}

func TestCmpBreaksWithSetString(t *testing.T) {
	x := &decimal.Big{}
	y := &decimal.Big{}
	x.SetString("1.57")
	y.Set(x)
	x.SetString("1.5")
	assert.Equal(t, -1, x.Cmp(y), fmt.Sprintf("%s < %s", x.String(), y.String()))
}

comunidadio avatar Mar 09 '23 09:03 comunidadio

As a workaround to allow reuse, calling SetUint64(0) before the second call to SetString makes it work as expected.

Checking the SetString implementation, it looks like it shall reset some fields before/during the string scan to avoid ending up in a mixed up state with the previous string (as per above the "1.57" becomes "1571.5" which makes it fail the comparison).

comunidadio avatar Mar 09 '23 13:03 comunidadio

SetString should be idempotent. I got the same issue and got very confused. Please fix this. Edit: also in the context of reducing allocation, but after benchmarking, seems like trying to reduce allocation is useless i.e. does not speed up things. Golang is probably very fast at allocating small chunks of memory in a tight loop like this.

vbmithr avatar Feb 23 '24 06:02 vbmithr