-
-
Notifications
You must be signed in to change notification settings - Fork 664
Fix Issue 22170: Add abs(), rem(), urem() and improve docs #22226
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request and interest in making D better, @Adityazzzzz! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
|
It would be good for a follow-up PR to add the overflow of signed/unsigned multiply that Iain mentioned in the ticket, if you are able to do so. |
9e10295 to
bdfec50
Compare
|
I have updated the changes in the PR, Please review |
Sure! I will take a look at that in a separate PR once this is merged |
druntime/src/core/int128.d
Outdated
| * c1 = operand 1 | ||
| * c2 = operand 2 | ||
| * c1 = operand 1 | ||
| * c2 = operand 2 |
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.
More space removal.
druntime/src/core/int128.d
Outdated
| * Params: | ||
| * c1 = dividend | ||
| * c2 = divisor | ||
| * c1 = dividend |
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.
More
druntime/src/core/int128.d
Outdated
| * c1 = dividend | ||
| * c2 = divisor | ||
| * Returns: | ||
| * remainder c1 % c2 |
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.
Indent.
druntime/src/core/int128.d
Outdated
| * c1 = dividend | ||
| * c2 = divisor | ||
| * Returns: | ||
| * remainder c1 % c2 |
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.
Again.
bdfec50 to
cb94465
Compare
|
Done! I've implemented the changes. |
| // Test rem/urem | ||
| assert(rem(C10, C3) == C1); // 10 % 3 = 1 | ||
| assert(urem(C10, C3) == C1); | ||
| assert(rem(Cm10, C3) == Cm1); // -10 % 3 = -1 |
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.
Perhaps add check that unsigned mod/remainder matches the % operator?
| assert(rem(Cm10, C3) == Cm1); // -10 % 3 = -1 | |
| assert(rem(Cm10, C3) == Cm1); // -10 % 3 = -1 | |
| assert(urem(Cm10, C3) == C0); // -10U % 3 = 0 |
Fixes Issue #22170.
Changes:
abs()helper function to core.int128.rem()(signed) andurem()(unsigned) wrapper functions.mul,div, andsarto clarify signed/unsigned behavior.