-
Notifications
You must be signed in to change notification settings - Fork 375
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(p/int256): "-0" Output When Calculating Basic Arithmetics #2750
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2750 +/- ##
==========================================
+ Coverage 60.21% 60.79% +0.57%
==========================================
Files 562 563 +1
Lines 75038 75985 +947
==========================================
+ Hits 45185 46192 +1007
+ Misses 26476 26398 -78
- Partials 3377 3395 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ToString
methodThere 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.
Just one comment. Looks good to me other than that.
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 think there's a problem at the root of this package; which is why is this not using two's complement?
The correct name for this package would actually be int257, because each Int is actually storing 257 bits of information, seeing as we separate the sign from the actual number.
If you were using two's complement, you wouldn't have problems like -0 to begin with, and addition/subtraction would Just Work without using any conditionals. So, if you're looking for improvements on this package, that's one place to look.
Aside from that, approach looks good. I made a suggestion to organize/optimize the result of Cmp.
If my recall correctly, this topic came up during the implementation, but we decided to follow another implementation that we were referencing, so that we eneded up setting the I definitely agree that we can eliminate a bunch of codes in current implementation. I'm currently preparing for a improvement of the 256-types, and I'll add this change as part of those modifications. |
Description
Div
functionContributors' checklist...
BREAKING CHANGE: xxx
message was included in the description