-
Notifications
You must be signed in to change notification settings - Fork 32
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
Allow wnaf to represent dense values #46
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR, and apologies for the slow review!
Blocked on fixing the width
bug (either in this PR or a separate one).
let mut acc = acc * 2; | ||
acc += *next as u128; |
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.
These lines panic in debug mode, which forbids integer overflow. I've replaced them with explicit wrapping operations, and added documentation to explain why it works.
let mut acc = acc * 2; | |
acc += *next as u128; | |
// If one of the least-significant w-NAF limbs is negative, `acc` may be large | |
// (due to the result being represented as a `u128`). `wrapping_mul` wraps at | |
// the boundary of the type; in this case, it has the effect of doubling the | |
// equivalent signed value: | |
// acc = -x = u128::MAX + 1 - x | |
// 2 * acc = 2 * (u128::MAX + 1 - x) | |
// = 2 * (u128::MAX + 1) - 2x | |
// = -2x as u128 | |
let acc = acc.wrapping_mul(2); | |
// Rust signed-to-unsigned casts add or subtract `T::MAX + 1` until the value | |
// fits into the new type. A wrapping addition of the new type is therefore | |
// equivalent to a wrapping subtraction of the magnitude of the original type. | |
acc.wrapping_add(*next as u128) |
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.
@str4d sidebar: you might consider adding the clippy::arithmetic_side_effects
lint
debug_assert!(window <= 64); | ||
debug_assert!(window < 64); |
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.
This seems like a separate issue, and fixing it in this PR is confusing. Rather than reducing the allowed values of window
, we should fix the bug by making width
a u128
, and adjusting the rest of the code accordingly.
@@ -144,6 +144,7 @@ pub(crate) fn wnaf_form<S: AsRef<[u8]>>(wnaf: &mut Vec<i64>, c: S, window: usize | |||
pos += window; | |||
} | |||
} | |||
wnaf.truncate(wnaf.len().saturating_sub(window - 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.
The purpose of this line is unclear, and it could accidentally lead to unintended truncation of meaningful limbs if a future refactor altered how the earlier logic works. Document this line.
} | ||
for w in 2..64 { | ||
for e in 0..=u16::MAX { | ||
let mut wnaf = vec![]; | ||
wnaf_form(&mut wnaf, e.to_le_bytes(), w); | ||
assert_eq!(e as u128, from_wnaf(&wnaf)); | ||
} | ||
} | ||
for w in 2..64 { | ||
for e in u128::MAX - 10000..=u128::MAX { | ||
let mut wnaf = vec![]; |
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.
wnaf_form
clears the given vector before using it, so this is faster (and also tests that clearing logic as a side-effect):
} | |
for w in 2..64 { | |
for e in 0..=u16::MAX { | |
let mut wnaf = vec![]; | |
wnaf_form(&mut wnaf, e.to_le_bytes(), w); | |
assert_eq!(e as u128, from_wnaf(&wnaf)); | |
} | |
} | |
for w in 2..64 { | |
for e in u128::MAX - 10000..=u128::MAX { | |
let mut wnaf = vec![]; | |
} | |
let mut wnaf = Vec::with_capacity(129); | |
for w in 2..64 { | |
for e in 0..=u16::MAX { | |
wnaf_form(&mut wnaf, e.to_le_bytes(), w); | |
assert_eq!(e as u128, from_wnaf(&wnaf)); | |
} | |
} | |
for w in 2..64 { | |
for e in u128::MAX - 10000..=u128::MAX { |
This PR fixes wnaf conversion for dense values which requires
len * 8 + 1
numbers rather thanlen * 8
. Also max window value is reduced to 63. It was before 64 and thenwidth = 1 << window
was overflowing.