optimize conversion #329
optimize conversion #329
Conversation
There was a problem hiding this comment.
Given implementation does not match the function description. BigInt should return integer component of decimal as BigInt and the proposed implementation just returns the underneeth coefficient scaled by 10^Exponent(). It's not the same thing. For example, for decimal 36235.1351 the current implementation return 36235 and yours implementation returns 362351351
| i := &big.Int{} | ||
| i.SetString(scaledD.String(), 10) | ||
| return i | ||
| return new(big.Int).Set(d.value) |
There was a problem hiding this comment.
| return new(big.Int).Set(d.value) | |
| scaledD := d.rescale(0) | |
| return new(big.Int).Set(scaledD.value) |
@mwoss this corrects your feedback. btw, why does rescale need to return a new Decimal when exp == d.exp? would expect with non-mutable API it'd be fine to return d
ofc, with d.rescale always allocating a fresh bigint, it'd be safe to return scaledD.value itself
There was a problem hiding this comment.
Thanks for working on that :D
btw, why does rescale need to return a new Decimal when exp == d.exp? would expect with non-mutable API it'd be fine to return d
If I remember correctly we have done it because, in a few places in the codebase, we expect that rescale always returns a new decimal, so we can safely perform operations on d.value. It could be improved for sure, but we would need to track down all those places and safely update the code and docs.
3 participants
No description provided.