Implements a pow method for Integers #11503

flaper87 · 2014-01-12T20:07:19Z

The patch adds the missing pow method for all the implementations of the
Integer trait. This is a small addition that will most likely be
improved by the work happening in #10387.

Fixes #11499

bill-myers · 2014-01-13T05:27:14Z

Well, this definitely needs to use https://en.wikipedia.org/wiki/Exponentiation_by_squaring instead of the linear exponentiation it uses in the commit.

Also, the algorithm is the same for any type that implements multiplication, so maybe it should be a default method in a trait inheriting from Mul<T, T> or a global function.

Then, the exponent should at least be uint instead of int (since exponentiation by a negative number is not defined for integers), and it probably should either be generic or also have a BigUint version.

Finally, maybe there should be support for negative exponentiation for Ratio.

huonw · 2014-01-13T05:37:57Z

src/libextra/num/bigint.rs

+    #[inline]
+    fn pow(&self, other: int) -> BigUint {
+        let m1 = (*self).clone();
+        let m2 = (*self).clone();


Is this clone even necessary?

You mean m2 or both of them?

Self cannot be de-referenced and BigUint is non-copiable, hence the 2 clones. Is there a better way to do this?

Yes, at the very least range(1, other).fold(m1, |acc, _| acc * *self) to avoid cloning to m2, or even range(0, other).fold(One::one(), |acc, _| acc * *self) to avoid both clones. (They should be implemented without clones when you convert it to exponentiation by squaring.)

flaper87 · 2014-01-13T07:56:48Z

@bill-myers thanks a lot for your comment. I'll apply your suggestions.

flaper87 · 2014-01-13T21:01:03Z

@huonw r?

bill-myers · 2014-01-14T02:05:44Z

Why isn't it possible to provide a single implementation for Mul<T, T> for any T instead of 4 copy&pasted implementations?

Also, it's better to use a loop instead of recursion (especially because that recursive version is not even tail recursive).

This should be an optimal implementation that avoids any unnecessary clone and any unnecessary instantiation of Zero or One (it's pseudocode):

Self: Mul<T, T>
fn square(&self) -> Self {self * self}
fn square_in_place(&mut self) -> Self {*self = self * self;}

Self: One+Mul<T, T>>, U: Zero+Shr<uint, U>
fn pow(&self, exp: &U) -> T {
    if(exp == Zero::zero()) {return One::one();}
    if(exp == One::one() {return self.clone();}
    // here exp >= 2, so we can start forming these and looking for a set bit after bit 0
    let mut i: U = exp >> 1;
    let mut v: T = self.square();
    let mut r: T;

    // find least significant bit set starting from bit 1, write its power in res, possibly multiplied by base if bit 0 is set
    // return if no other bits set
    // otherwise point i and v to the bit after that
    loop {
        if(i.is_odd()) {
            break;
        }
        i >>= 1;
        v.square_in_place();
    }
    i >>= 1;
    if(i.is_zero()) {return if exp.is_odd() {self * v} else {v};}
    if exp.is_odd() {
        r = self * v;
        v.square_in_place();
    } else {
        let v2 = v.square();
        r = v;
        v = v2;
    }

    // multiply in all other bits
    loop {
        if(i.is_odd()) {
            r *= v;
            i >>= 1;
            if(i.is_zero()) {return res;}
        } else {
            i >>= 1;
        }
        v.square_in_place();
    }
}
}

bill-myers · 2014-01-14T02:46:25Z

BTW, that's still horribly inefficient for general exponents.

The correct solution is not really to shift right, but rather to use a generic indexed "find next set bit" operation to iterate over set bits (which requires defining such a trait).

flaper87 · 2014-01-14T09:58:45Z

@bill-myers thanks again for your comments. I should've definitely thought about the recursion issues. Let me update the patch with something along the lines of your pseudo-code.

flaper87 · 2014-01-15T01:16:53Z

@bill-myers @huonw r?

huonw · 2014-01-15T04:18:53Z

src/libstd/num/mod.rs

+    fn pow<U: Eq+Zero+One+BitAnd<U, U>+Shr<U, U>>(&self, exp: U) -> Self {
+        let one: U = One::one();
+
+        if exp.is_zero() { return One::one(); }


Can't this just return one?

oppps, yes!

Actually I'm wrong: the return type is Self but one is U.

yeah, noticed it right after my comment. :)

huonw · 2014-01-15T04:25:28Z

It looks better now with squaring, but... the implementation seems rather complicated, I'll play around with it a bit later.

huonw · 2014-01-15T04:26:04Z

src/libstd/num/mod.rs

+    fn square(&self) -> Self {self * *self}
+
+    /**
+     * Elevates a type implementing `Mul` to the


We now normally write these doc comments with /// rather than /** */.

flaper87 · 2014-01-15T06:42:29Z

I'm pretty sure it can be implemented with a single loop. I'll give it another go.

flaper87 · 2014-01-15T10:16:44Z

@huonw r?

huonw · 2014-01-15T10:26:34Z

src/libstd/num/mod.rs

+        let mut v: Self;
+        let mut r: Self = self_one;
+
+        if((i & one) == one) {


Also, what is this if for? It's to avoid cloning self, right? It'd be good to have a comment saying this.

damn! Changed! Thnx

Correct, it's to avoid cloning self.

flaper87 · 2014-01-15T13:23:48Z

@huonw r?

huonw · 2014-01-15T13:28:49Z

src/libstd/num/mod.rs

+    #[inline]
+    fn square(&self) -> Self {self * *self}
+
+    ///


Normally we don't have these first and last ones.

huonw · 2014-01-15T13:32:19Z

As I said on IRC, I don't like adding a new trait just for method-call-syntax, and I especially don't like adding such a trait that the user has to actually implement themselves. If we are going to keep the trait, it would better to have it also have

impl<T: One + Clone + Mul<T, T>> Power for T {}

inside libstd.

flaper87 · 2014-01-15T13:34:39Z

I understand your concern. However, the T.pow(uint) usage is more convenient than num::pow(T, uint).

@brson @alexcrichton @bill-myers thoughts?

huonw · 2014-01-15T13:45:09Z

Advantages of the trait: method call syntax.

Disadvantages:

very poorly documented (especially extension traits like this)
either, yet another trait in the prelude (which reserves valuable method names)
or, requiring use std::num::Power; to call anyway (little difference to use std::num::pow;)

flaper87 · 2014-01-15T13:53:45Z

Those are all good points. Thanks!

TBH, I'm ok with both, I do prefer the call syntax of the current implementation. IMHO, we could also make it part Mul. (Unless, I'm missing something). Otherwise, I'll just STFU and implement it as a standalone function. 😄

huonw · 2014-01-16T00:55:55Z

IMHO, we could also make it part Mul. (Unless, I'm missing something)

I don't think Mul has enough information about the type to work: in particular, it doesn't require the type to also implement One or Clone.

huonw · 2014-01-16T00:56:10Z

(Accidental close, sorry.)

flaper87 · 2014-01-16T07:40:21Z

Right, we actually talked about that :D Ok, now I'm convinced too. I'll change the implementation to a standalone function. Thanks

flaper87 · 2014-01-16T13:42:21Z

@huonw r?

huonw · 2014-01-16T14:01:33Z

src/libstd/num/mod.rs

+/// Raises a value to the power of exp, using
+/// exponentiation by squaring.
+///
+/// It could be optimized by using indexed access to


I don't think this comment is relevant when exp: uint, and it would be good to have an example.

flaper87 · 2014-01-16T14:35:47Z

@huonw r?

flaper87 · 2014-01-16T17:08:12Z

@huonw r?

flaper87 · 2014-01-17T07:59:03Z

@huonw r? :)

The patch adds a `pow` function for types implementing `One`, `Mul` and `Clone` trait. The patch also renames f32 and f64 pow into powf in order to still have a way to easily have float powers. It uses llvms intrinsics. The pow implementation for all num types uses the exponentiation by square. Fixes bug rust-lang#11499

The patch adds the missing pow method for all the implementations of the Integer trait. This is a small addition that will most likely be improved by the work happening in #10387. Fixes #11499

fix filter_map_bool_then with a bool reference changelog: [`filter_map_bool_then`]: Fix the incorrect autofix when the `bool` in question is a reference. fix rust-lang#11503

…=Jarcho [`filter_map_bool_then`]: include multiple derefs from adjustments In rust-lang#11506 this lint was improved to suggest one deref if the bool is behind references (fixed the FP rust-lang#11503), however it might need multiple dereferences if the bool is behind multiple layers of references or custom derefs. E.g. `&&&bool` needs `***b`. changelog: [`filter_map_bool_then`]: suggest as many dereferences as there are needed to get to the bool

huonw reviewed Jan 13, 2014
View reviewed changes

huonw reviewed Jan 15, 2014
View reviewed changes

src/libstd/num/mod.rs

#[inline]

fn square(&self) -> Self {self * *self}

///

Copy link

Member

huonw Jan 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we don't have these first and last ones.

huonw closed this Jan 16, 2014

huonw reopened this Jan 16, 2014

huonw reviewed Jan 16, 2014
View reviewed changes

bors closed this Jan 18, 2014

bors merged commit ed7e576 into rust-lang:master Jan 18, 2014

bill-myers mentioned this pull request Jan 23, 2014

Added powm (power modulo) to the std::num module. #11735

Closed

Implements a pow method for Integers #11503

Implements a pow method for Integers #11503

Conversation

flaper87 commented Jan 12, 2014

bill-myers commented Jan 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Jan 13, 2014

flaper87 commented Jan 13, 2014

bill-myers commented Jan 14, 2014

bill-myers commented Jan 14, 2014

flaper87 commented Jan 14, 2014

flaper87 commented Jan 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Jan 15, 2014

Choose a reason for hiding this comment

flaper87 commented Jan 15, 2014

flaper87 commented Jan 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Jan 15, 2014

Choose a reason for hiding this comment

huonw commented Jan 15, 2014

flaper87 commented Jan 15, 2014

huonw commented Jan 15, 2014

flaper87 commented Jan 15, 2014

huonw commented Jan 16, 2014

huonw commented Jan 16, 2014

flaper87 commented Jan 16, 2014

flaper87 commented Jan 16, 2014

Choose a reason for hiding this comment

flaper87 commented Jan 16, 2014

flaper87 commented Jan 16, 2014

flaper87 commented Jan 17, 2014