-
Notifications
You must be signed in to change notification settings - Fork 191
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
Implement operators with 128bits #64
Conversation
dignifiedquire
commented
Jul 22, 2018
- Depends on chore: run rustfmt on all code #63
- This is the second part to Add support for 128-bit primitives #40
Please rebase on the rebased+merged #63. |
aadfda0
to
6ed4f0e
Compare
6ed4f0e
to
3593928
Compare
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.
Nice job! Just left a few small requests.
Despite all the macros, there's still a lot of copy-paste with only slight edits, but that's not your fault. Maybe someday we'll figure out how to refactor all of this with less duplication...
if other != 0 { | ||
if self.data.len() == 0 { | ||
self.data.push(0); | ||
} | ||
|
||
let carry = __add2(&mut self.data, &[other]); | ||
let carry = __add2(&mut self.data, &[other as BigDigit]); |
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.
Let's use From
or Into
here, just to future-proof that this is never lossy, e.g. &[other.into()]
(ditto for all of the similar places with a lossless cast as BigDigit
.)
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 are a bunch of places in here already that used as
instead of from/into
. Should I just convert all of them, or is there any potential drawback?
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.
Hmm, we can leave that for future cleanup, but let's at least do that before (or during) the BigDigit
upgrade.
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.
created a tracking issue for this: #66
self.data.push(0); | ||
} | ||
__add2(&mut self.data, &[d, c, b, a]) | ||
} else { |
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.
Let's debug_assert!(b > 0)
here, as a self-check/documentation that smaller values were caught as u64
already.
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.
fixed
src/biguint.rs
Outdated
bits += big_digit::BITS; | ||
} | ||
|
||
println!("{:?} -> {}", self, ret); |
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.
Stray debug print?
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.
fixed
Thanks! bors r+ |
👎 Rejected by code reviews |
bors r+ |
Build succeeded |
62: Finish i128 support r=cuviper a=dignifiedquire Depends on #63 and #64 Implements `BigDigit = u64` ~~behind a feature flag `u64_digit`~~ for all 64-bit targets with `u128`. TODOs - [x] find a way to make the new methods that take `BigDigit`s for constructing `BigUint`s private - [x] Fix `modpow`, the last broken test. Closes #40 Co-authored-by: dignifiedquire <dignifiedquire@gmail.com> Co-authored-by: Josh Stone <cuviper@gmail.com>