-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add QuoRem and Mod #27
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27 +/- ##
==========================================
- Coverage 95.97% 95.94% -0.04%
==========================================
Files 5 5
Lines 1789 1849 +60
==========================================
+ Hits 1717 1774 +57
- Misses 48 50 +2
- Partials 24 25 +1 ☔ View full report in Codecov by Sentry. |
f612c2d to
f5ee09b
Compare
f5ee09b to
3580b35
Compare
| q, r, err := tryQuoRemU128(d, e) | ||
| if err == nil { | ||
| return q, r, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method return an error that is not about an overflow?
I'm asking because I find strange arrow is totally in your when not nil. Which means not no matter arrow the process will continue the same way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I get what you mean. Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When no error, the result is considered OK.
So you return it, so it's good.
But then, you assume implicitly that any error is about overflow.
But it could be something else.
I would have expected something like this
| q, r, err := tryQuoRemU128(d, e) | |
| if err == nil { | |
| return q, r, nil | |
| } | |
| q, r, err := tryQuoRemU128(d, e) | |
| if err == nil { | |
| return q, r, nil | |
| } | |
| if !errors.Is(err, errOverflow) { | |
| return Decimal{}, Decimal{}, err | |
| } |
Or this
| q, r, err := tryQuoRemU128(d, e) | |
| if err == nil { | |
| return q, r, nil | |
| } | |
| q, r, err := tryQuoRemU128(d, e) | |
| switch { | |
| case errors.Is(err, errOverflow): | |
| // OK, let's continue with bigint | |
| case err == nil { | |
| // we computed it | |
| return q, r, nil | |
| default: | |
| // anything else must be reported | |
| return Decimal{}, Decimal{}, err | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only returns overflow error so it's fine.
errors.Is is also much slower than comparing errors directly. I think it should be avoided to not affect the performance
goos: linux
goarch: amd64
pkg: github.com/quagmt/udecimal
cpu: Intel(R) Core(TM) i9-14900HX
BenchmarkErrorIs/errors.Is-32 100420094 11.79 ns/op 0 B/op 0 allocs/op
BenchmarkErrorIs/sentinel_error-32 1000000000 0.1929 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/quagmt/udecimal 2.052s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only errOverflow is returned everywhere, maybe you could remove the error logic and return bool that will be true if it overflows.
Because it's not an error then, but a flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean makes more sense in this case tbh, however using bool as a return value needs some more annotations/comments or named return values (I'm not a big fan) to explain it. I found errOverflow is self-explanatory and easier to understand. Do you have any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, what about this
| q, r, err := tryQuoRemU128(d, e) | |
| if err == nil { | |
| return q, r, nil | |
| } | |
| q, r, err := tryQuoRemU128(d, e) | |
| switch { | |
| case err == nil { | |
| // we computed it | |
| return q, r, nil | |
| case err == errOverflow: // the only expected error | |
| // OK, let's continue with bigint | |
| default: | |
| // anything else must be reported | |
| return Decimal{}, Decimal{}, err | |
| } |
| //nolint:gosec | ||
| r := u128{hi: a[1], lo: a[0]}.Rsh(uint(n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the nolint:gosec is about converting n to uint.
I would add a comment explaining why it's OK
I tend to use nolintlint linter with require - explanation:true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's straightforward that n=bits.LeadingZero64 can be safely converted to uint, isn't it? I agree that nolint with an explanation is useful, tho. Will add it in a different PR.
Description
QuoRemandMod. The implementation is similar toshopsping/decimaland C's fmod.