num-integer icon indicating copy to clipboard operation
num-integer copied to clipboard

Integer::extended_gcd should not permit unsigned value.

Open aobatact opened this issue 3 years ago • 5 comments

This code fails to get the y value, with the error 'attempt to subtract with overflow'.

#[test]
fn extended_gcd_test(){
  use num_integer::Integer;
  let _x = 10.extended_gcd(&4);
  let _y = 10_u32.extended_gcd(&4);
}

aobatact avatar Apr 26 '21 11:04 aobatact

May be we can require Self: Signed for extended_gcd

aobatact avatar Apr 26 '21 11:04 aobatact

Yeah, we do require Signed on extended_gcd_lcm, but that was overlooked on extended_gcd. Unfortunately it requires a breaking change to add that constraint.

cuviper avatar Apr 26 '21 22:04 cuviper

Alternatively, it's possible to support extended_gcd for unsigned integers as described here: https://stackoverflow.com/q/67097428/1299394, which would avoid a breaking change.

Edit: because x and y must both be positive for unsigned integers, you'd need to modify the check() function used for testing - instead of assert_eq!(gcd, x * a + y * b); it would need to be something like:

assert_eq!(
  gcd,
  (x * a + y * b) % (a * b),
);

aj-r avatar Jan 08 '24 00:01 aj-r

@aj-r interesting, thanks! I'm open to "fixing" unsigned with that kind of implementation, as long as we're careful to document the difference -- and ideally how to reconstruct what the signed result would have been.

cuviper avatar Jan 08 '24 20:01 cuviper

Maybe we could then drop Signed from extended_gcd_lcm -- but I think that's technically a breaking change for anyone that overrode that method, if they depended on the conditional Signed constraint in their impl. What a mess...

cuviper avatar Jan 08 '24 20:01 cuviper