add mul method for BigInteger
#772
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tcoratger
requested review from
z-tech,
Pratyush and
mmagician
and removed request for
a team
Pratyush
reviewed
Feb 5, 2024
Pratyush
requested changes
Feb 5, 2024
Pratyush
reviewed
Feb 5, 2024
Pratyush
reviewed
Feb 5, 2024
| @@ -49,6 +50,33 @@ fn biginteger_arithmetic_test<B: BigInteger>(a: B, b: B, zero: B) { | |||
| let mut a_plus_a = a; | |||
| a_plus_a.add_with_carry(&a); // Won't assert anything about carry bit. | |||
| assert_eq!(a_mul2, a_plus_a); | |||
|
|
|||
| // a * 1 = a | |||
There was a problem hiding this comment.
Let's also add some tests to test-templates, if appropriate?
There was a problem hiding this comment.
For these implementations around BigInt, all unit tests can be found in this file:
- So I would say that for the moment there is not necessarily a need to outsource the tests to
test-templatesno? - I added a
maxparameter to try to test with a slightly wider coverage of all the multiplication features - If you want I can also add another function here that tests something specifically for multiplication? But it seems to me that with the tests here we are pretty well covered.
- To avoid mixing the PRs, if you want we can add a new issue to migrate all the tests that are here to
test-templatesand try to put an even broader conversion on all the functions of the implementation, I don't know if you see any interest in it?
Co-authored-by: Pratyush Mishra <[email protected]>
Pratyush
reviewed
Feb 7, 2024
Comment on lines
+446
to
+464
| #[inline] | ||
| fn mul_low(&mut self, other: &Self) { | ||
| if self.is_zero() || other.is_zero() { | ||
| *self = Self::zero(); | ||
| return; | ||
| } | ||
|
|
||
| let mut res = Self::zero(); | ||
| let mut carry = 0; | ||
|
|
||
| for i in 0..N { | ||
| for j in 0..(N - i) { | ||
| res.0[i + j] = mac_with_carry!(res.0[i + j], self.0[i], other.0[j], &mut carry); | ||
| } | ||
| carry = 0; | ||
| } | ||
|
|
||
| *self = res | ||
| } |
There was a problem hiding this comment.
Suggested change
| #[inline] | |
| fn mul_low(&mut self, other: &Self) { | |
| if self.is_zero() || other.is_zero() { | |
| *self = Self::zero(); | |
| return; | |
| } | |
| let mut res = Self::zero(); | |
| let mut carry = 0; | |
| for i in 0..N { | |
| for j in 0..(N - i) { | |
| res.0[i + j] = mac_with_carry!(res.0[i + j], self.0[i], other.0[j], &mut carry); | |
| } | |
| carry = 0; | |
| } | |
| *self = res | |
| } | |
| #[inline] | |
| fn mul_low(&self, other: &Self) -> Self { | |
| if self.is_zero() || other.is_zero() { | |
| *self = Self::zero(); | |
| return; | |
| } | |
| let mut res = Self::zero(); | |
| let mut carry = 0; | |
| for i in 0..N { | |
| for j in 0..(N - i) { | |
| res.0[i + j] = mac_with_carry!(res.0[i + j], self.0[i], other.0[j], &mut carry); | |
| } | |
| carry = 0; | |
| } | |
| res | |
| } |
There was a problem hiding this comment.
(For consistency, the operations should all return values. If we want in place operations, we should mark those explicitly)
There was a problem hiding this comment.
Sure no problem, I've just followed the convention of other operations here like mul2 or muln.
Just did a follow up PR here: #775
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces a new
mulmethod for theBigIntegertrait. Themulmethod allows efficient in-place multiplication of twoBigIntegervalues, providing a convenient and performant way to compute the product of two instances.This enhancement complements the existing set of arithmetic operations supported by the
BigIntegertrait, contributing to the versatility and usability of the library.Should be part of #645.
Pendingsection inCHANGELOG.mdFiles changedin the GitHub PR explorer