Skip to content
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 modpow for exp=1 #113

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Fix modpow for exp=1 #113

merged 1 commit into from
Oct 9, 2019

Conversation

youknowone
Copy link
Contributor

Unfortunately, there was cases modulus never happened

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this is a regression in 0.2.3 following the optimizations in #93. While I expect a 1 exponent would be unusual, we should still fix this, and I appreciate that you're proactive here. Let's get this sorted out and published!

src/biguint.rs Outdated
@@ -2285,6 +2285,9 @@ fn plain_modpow(base: &BigUint, exp_data: &[BigDigit], modulus: &BigUint) -> Big

let mut exp_iter = exp_data[i + 1..].iter();
if exp_iter.len() == 0 && r.is_one() {
if &base > modulus {
base %= modulus;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to normalize this above where we create a base.clone(), just use base % modulus instead. The implementation of that % operator already checks if the value is less than the modulus too.

@@ -81,6 +81,7 @@ mod biguint {
check_modpow::<u32>(0, 15, 11, 0);
check_modpow::<u32>(3, 7, 11, 9);
check_modpow::<u32>(5, 117, 19, 1);
check_modpow::<u32>(20, 1, 2, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a similar test with an odd modulus too, so that corner case is not missed. (But I think that implementation is already OK.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I added api surface test for small values below

@youknowone youknowone force-pushed the modpow-exp1 branch 2 times, most recently from 73731c8 to 904f057 Compare October 9, 2019 07:45
@cuviper
Copy link
Member

cuviper commented Oct 9, 2019

Thanks!

bors r+

bors bot added a commit that referenced this pull request Oct 9, 2019
113: Fix modpow for exp=1 r=cuviper a=youknowone

Unfortunately, there was cases modulus never happened

Co-authored-by: Jeong YunWon <jeong@youknowone.org>
@bors
Copy link
Contributor

bors bot commented Oct 9, 2019

Build succeeded

@bors bors bot merged commit 00042ec into rust-num:master Oct 9, 2019
@youknowone youknowone deleted the modpow-exp1 branch October 10, 2019 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants