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

Feature: more comparison operations with BigInts #430

Open
Seltyk opened this issue Dec 3, 2023 · 6 comments
Open

Feature: more comparison operations with BigInts #430

Seltyk opened this issue Dec 3, 2023 · 6 comments

Comments

@Seltyk
Copy link

Seltyk commented Dec 3, 2023

Right now, the only comparison operation between BigInt types is PartialOrd<Rhs = Self>. There are times, however, that other comparisons are helpful, especially comparisons against primitive integer types. Most of these comparisons could be Ord in the case of BigInt, BigUint, and BigRational, though comparisons involving complex numbers could not be Ord. Even comparisons across differently-signed bigint types are trivial, simply adding an O(1) sign check before the vector-based comparing.

Admittedly, the primitive type comparison can be done already (e.g. x >= BigUint::from(8u64) or x < 16u64.into()), but this is inefficient in terms of time and memory.

Naturally, the reverse (primitive cmp bigint) would also be helpful, though thankfully implementing that will be trivial when the above implementation is made; it's the same source code with some text swapped around. I don't know much about macros, but this sounds like a job for macros.

@cuviper
Copy link
Member

cuviper commented Dec 3, 2023

We had this in development with rust-num/num-bigint#136, but I ran into type inferences in issue rust-num/num-bigint#150, and reverted in rust-num/num-bigint#151.

The inference issue is pretty nasty, because it even breaks code that would otherwise have nothing to do with bigints, just by the compiler seeing the additional trait implementations present at all. And I fully expect it would have the same result if we extended others like Ratio too.

@cmrschwarz
Copy link

If we cannot have these as PartialOrd, can we maybe get regular member functions like cmp_i64 [...] (or a generalized cmp_foreign) or something? Implementing these by hand is pretty tedious and error prone,
and eating a memory allocation (and deallocation) every time you compare against a bog standard i64 (mybigint.cmp(&BigInt::from(myi64))) is pretty bad.

@cuviper
Copy link
Member

cuviper commented Nov 25, 2024

can we maybe get regular member functions like cmp_i64 [...] (or a generalized cmp_foreign) or something?

Yes, I think we could reasonably have something comparing a T: PrimInt, using its NumCast or ToPrimitive supertraits in the implementation. Those are fallible though, so it still might not handle a case like an external u256 with a value that can't fit in an actual primitive integer.

@cmrschwarz
Copy link

Those are fallible though

That doesn't sound right to me. A comparison between two integers should never fail, such an API seems really frustrating for users. What are you supposed to do if it fails?

Why not e.g. have a trait like PrimIntCmp<T: PrimInt> that essentially uses the existing implementation to provide a prim_int_cmp method?
If we wanted we could even have a FloatCmp<T: Float>, although that would would actually have to return an Option because of NaNs.

@cuviper
Copy link
Member

cuviper commented Nov 25, 2024

We could make a new trait that's more capable, but I was thinking about what is possible with existing traits. So with T: PrimInt (and PrimInt: NumCast), we could convert the unknown T to i128/u128 and do the comparison -- but that NumCast conversion returns an Option. Thus a u256::MAX couldn't be compared that way.

@cmrschwarz
Copy link

I see what you mean. I guess that's clever and keeps the amount of code & traits down, although the usability would be a bit worse.

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

No branches or pull requests

3 participants