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

implement Pow for BigInt #106

Closed
programmerjake opened this issue Aug 23, 2019 · 2 comments · Fixed by #137
Closed

implement Pow for BigInt #106

programmerjake opened this issue Aug 23, 2019 · 2 comments · Fixed by #137

Comments

@programmerjake
Copy link

programmerjake commented Aug 23, 2019

Pow<T> is not implemented for BigInt, only for &BigInt. This causes problems for using Pow on Ratio<BigInt> since Ratio<T>'s Pow implementation requires T: Pow.

@cuviper
Copy link
Member

cuviper commented Aug 27, 2019

Hmm, I see.

This was sort of intentional, because having Pow<_> for BigInt by value means that x.pow(y) tries to consume x, if it's not already a reference. You need an ugly (&x).pow(y) to avoid that.

We probably should have made Pow<_> for &Ratio<T> require &T: Pow<_> instead, but that will be a breaking change now.

@programmerjake
Copy link
Author

programmerjake commented Aug 27, 2019

Hmm, I see.

This was sort of intentional, because having Pow<_> for BigInt by value means that x.pow(y) tries to consume x, if it's not already a reference. You need an ugly (&x).pow(y) to avoid that.

We probably should have made Pow<_> for &Ratio<T> require &T: Pow<_> instead, but that will be a breaking change now.

I think we should just implement Pow<_> for both BigInt and &'_ BigInt, just like all the other arithmetic operations, that way users who don't want to lose the input value can just call (&x).pow(_), exactly the same as all the other arithmetic operations.

For consistency's sake, I think we should implement all of:

impl Pow<_> for BigInt { ... }
impl Pow<_> for &'_ BigInt { ... }
impl Pow<&'_ _> for BigInt { ... }
impl<'l, 'r> Pow<&'r _> for &'l BigInt { ... }
impl Pow<_> for BigUint { ... }
impl Pow<_> for &'_ BigUint { ... }
impl Pow<&'_ _> for BigUint { ... }
impl<'l, 'r> Pow<&'r _> for &'l BigUint { ... }

if that isn't acceptable, an alternate solution is:

trait PowWorkaround<Rhs> {
    type Output;
    fn pow_workaround(self, rhs: Rhs) -> Self::Output;
}

impl<Rhs> PowWorkaround<Rhs> for BigInt // also BigUint
where
    for<'a> &'a Self: Pow<Rhs, Output=Self>
{
    type Output = Self;
    fn pow_workaround(self, rhs: Rhs) -> Self {
        (&self).pow(rhs)
    }
}

impl<T, Rhs> PowWorkaround<Rhs> for T
where
    Self: Pow<Rhs>
{
    type Output = <Self as Pow<Rhs>>::Output;
    fn pow_workaround(self, rhs: Rhs) -> Self::Output {
        Pow::pow(self, rhs)
    }
}

impl<T: Integer + Clone + PowWorkaround<_, Output=T>> impl Pow<_> for &'_ Ratio<T> {
    ...
}

bors bot added a commit that referenced this issue Mar 3, 2020
137: Implement `Pow` by value r=cuviper a=cuviper

We already had `Pow` for references, so the default call doesn't consume
your allocation, but this wasn't sufficient for uses like `BigRational`.
Now we do implement it by value, and have also added a new inherent
method `pow(&self, exp: u32)` that takes precedence. That exponent type
matches the same method on primitive integers, and is likely sufficient
for most use cases.

Fixes #106

Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors bors bot closed this as completed in 83bd68c Mar 3, 2020
@bors bors bot closed this as completed in #137 Mar 3, 2020
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

Successfully merging a pull request may close this issue.

2 participants