fix: relax the constraints of floor_sum #99
Conversation
| @@ -186,24 +186,20 @@ pub fn crt(r: &[i64], m: &[i64]) -> (i64, i64) { | |||
| /// assert_eq!(math::floor_sum(6, 5, 4, 3), 13); | |||
| /// ``` | |||
| pub fn floor_sum(n: i64, m: i64, mut a: i64, mut b: i64) -> i64 { | |||
| assert!(0 <= n && n < 1i64 << 32); | |||
| assert!(1 <= m && m < 1i64 << 32); | |||
| let mut ans = 0; | |||
There was a problem hiding this comment.
Since overflows occur in C++ without special handling, they use unsigned long long here to extend the range of values returned correctly; implementing this in Rust would require a lot of casting (as u64) and functions like u64::overflowing_sub. Since this is cumbersome, I've written the internal computation in i64, but this may have led to more cases of overflowing only in the middle of the computation. Which would be better?
There was a problem hiding this comment.
Why not using wrapping_* variants?
src/math.rs
Outdated
| pub fn floor_sum(n: i64, m: i64, mut a: i64, mut b: i64) -> i64 { | ||
| assert!(0 <= n && n < 1i64 << 32); |
There was a problem hiding this comment.
Could you just replace it with something like matches!(n, 0..1 << 32)? This is slightly better than (0..1 << 32).contains(&n) because it does not involve taking a reference to n, which may avoid constant-factor performance degradation.
There was a problem hiding this comment.
Since the implementation of matches! macro seems to be insufficient, it causes error on parsing. Should I replace it with (0..1<<32).contains(&n) ?
% cargo build
Compiling ac-library-rs v0.1.0 (/home/user/xxx/ac-library-rs)
error: no rules expected the token `<<`
--> src/math.rs:190:33
|
190 | assert!(matches!(n, 0..1i64 << 32));
| ^^ no rules expected this token in macro call
error: no rules expected the token `<<`
--> src/math.rs:191:33
|
191 | assert!(matches!(m, 1..1i64 << 32));
| ^^ no rules expected this token in macro call
error: aborting due to 2 previous errors
error: could not compile `ac-library-rs`.
To learn more, run the command again with --verbose.
src/math.rs
Outdated
| pub fn floor_sum(n: i64, m: i64, mut a: i64, mut b: i64) -> i64 { | ||
| assert!(0 <= n && n < 1i64 << 32); | ||
| assert!(1 <= m && m < 1i64 << 32); |
| pub fn floor_sum(n: i64, m: i64, mut a: i64, mut b: i64) -> i64 { | ||
| assert!((0..1i64 << 32).contains(&n)); | ||
| assert!((1..1i64 << 32).contains(&m)); | ||
| let mut ans = 0; |
There was a problem hiding this comment.
Let's wrap the variables with std::num::Wrapping<u64>. Note that if you shadow the variables, you need to check if a and b are negative beforehand.
| /// `sum_{i=0}^{n-1} floor((ai + b) / m) (mod 2^64)` | ||
| /* const */ | ||
| #[allow(clippy::many_single_char_names)] | ||
| pub(crate) fn floor_sum_unsigned(mut n: u64, mut m: u64, mut a: u64, mut b: u64) -> u64 { |
There was a problem hiding this comment.
Let this function take Wrapping<u64> instead of u64 to avoid complication; it's an internal function anyway.
|
@TonalidadeHidrica @mizar How about merging this PR now and fix it later? |
ref #91
this PR contains following two changes: