Fix i256 checked multiplication #2818
Conversation
| low, | ||
| high: (high as i128).checked_add(hl)?.checked_add(lh)?, | ||
| }) | ||
| let high: i128 = high.checked_add(hl)?.checked_add(lh)?.try_into().ok()?; |
There was a problem hiding this comment.
I may miss it. But don't we need to count in self.high multiplies other.high?
There was a problem hiding this comment.
Nice spot, the result of the high products is necessarily overflow, and so can simply be ignored for the wrapping version, but must be checked here
|
There be more bugs 😢 |
|
I plan to give this another once over with fresh eyes in the morning, and then get this in. |
|
Benchmark runs are scheduled for baseline = 24e9b7c and contender = a0a263f. a0a263f is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
| i256::from_parts(0, 0), | ||
| i256::from_parts(0, 1), | ||
| i256::from_parts(0, -1), | ||
| i256::from_parts(u128::MAX, 1), |
There was a problem hiding this comment.
thanks -- this is much easier to ensure coverage
Which issue does this PR close?
Part of #2637
Rationale for this change
The previous version was failing to correctly detect overflow.
What changes are included in this PR?
There may be a smart way to detect signed overflow correctly, but trying to think about this just did my head in. The simple solution is to take the absolute value, perform the arithmetic and convert back. I think this is fine, and if someone has a use-case where this becomes a major performance bottleneck we can revisit.
Are there any user-facing changes?
No