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

fix the rounding bug in roundShortest method #161

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

dreamerjackson
Copy link
Contributor

optimization roundShortest method for effectiveness
which had apply in go source code: go-source-strconv

@mwoss
Copy link
Member

mwoss commented Jan 2, 2020

Thanks for your contribution @dreamerjackson. Before diving in code review, could you provide proper benchmarks too? So all be aware of the importance of this optimization.

@dreamerjackson
Copy link
Contributor Author

@mwoss soory, i check it and find it's about a bug in rounding shortest. the fix and detail description is there:go-commit
i add the test in Most recent commit

@dreamerjackson
Copy link
Contributor Author

@cmars @chrismrivera @soyangel the fix can merge?

@mwoss
Copy link
Member

mwoss commented Jul 5, 2020

Hi @dreamerjackson. I kinda forgot about your PR, I apologize. Could you rename the pull request title to match code content? As you said it's about fixing the bug, not optimization at all. Below I provided the additional benchmarks.

goos: windows
goarch: amd64
pkg: github.com/shopspring/decimal

Benchmark name Loops Time per op Memory per op Alloc per op
BenchmarkNewFromFloat-8 2350124 506 ns/op 48 B/op 2 allocs/op
BenchmarkNewFromFloatBugFix-8 2228163 539 ns/op 48 B/op 2 allocs/op

@dreamerjackson dreamerjackson changed the title feat: optimization roundShortest method fix the rounding bug in roundShortest method Jul 6, 2020
@dreamerjackson
Copy link
Contributor Author

@mwoss i have fixed the title description,please review it again , thx~

Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

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

Properly vendorder, ready to merge.

@mwoss mwoss merged commit 867ed12 into shopspring:master Jul 7, 2020
fairyhunter13 added a commit to fairyhunter13/decimal that referenced this pull request Jul 12, 2020
* fix: fix rounding in FormatFloat fallback path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants