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

Generic exp for pow and checked_pow #300

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mtshr
Copy link

@mtshr mtshr commented Nov 25, 2023

Due to the exp being usize, the current implementation has been inconsistent with core implementation of pow and checked_pow taking u32 for exp. This has also inhibited implementation of some Pow<u*> for primitive integers.

This pull request generalizes exp to use unsigned primitive integer for the sake of the implementation of lacking Pow<u*> and more importantly, I guess, the consistency with core pow and checked_pow utilizing u32, for the use of architecture dependently sized type, namely usize here, for arithmetic function does not sound reasonable.

Although this change should be a minor change as usize does implements PrimInt + Unsigned, please note that the existing crate depending on the functions in question may come to be in need of adding some type notation, as {integer} is assumed to be i32.

num_traits::pow(7, 7);         // ng
num_traits::pow(7, 7usize);    // ok

(prim_int $t:ty) => {
pow_impl!($t, u8);
pow_impl!($t, u16);
pow_impl!($t, u32, u32, <$t>::pow);
Copy link
Author

Choose a reason for hiding this comment

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

Utilizes the default implementation of pow for exp with u32

if exp == 0 {
pub fn pow<T, U>(mut base: T, mut exp: U) -> T
where
T: Clone + One,
Copy link
Author

Choose a reason for hiding this comment

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

I removed T: Mul<T, Output = T> because One needs it.

@cuviper
Copy link
Member

cuviper commented Nov 28, 2023

It is an unfortunate historical difference, but I think the {integer} inference loss is probably not worth it when the more flexible Pow<RHS> already exists. Do you need an equivalent CheckedPow?

@mtshr
Copy link
Author

mtshr commented Nov 29, 2023

Thank you for taking a look.

I think the {integer} inference loss is probably not worth it when the more flexible Pow<RHS> already exists.

I am sorry that my English ability is not sufficient, but am I right in interpreting you think it is too much to change exp: usize to general one and therefore are not in favor of the change of the pull request? What I give more importance here is actually not the fact that the user can implement their own pow with Pow<RHS>, but the reusability of the library's existing code and mitigating the concern about type size. After implementing Clone (and preferably Copy for the case I am describing at the moment considering the cost) + One for their own struct, the user will be able to just call num_traits::pow without writing their own pow. Also for example, for Wrapping<T> or something similar, the user can reasonably take exp >= 2^32, but always having to take usize's actual size into account leads to implementing their own pow and in the end the user cannot benefit much from pub pow.

Do you need an equivalent CheckedPow?

I know others may have a interest in one (ref #254) and I am not against the idea to add one. I consider making a pull request once the idea of generalizing exp is accepted, for I do not want to yield another FIXME.

num-traits/src/pow.rs

Lines 26 to 29 in 61d9a1b

// FIXME: these should be possible
// pow_impl!($t, u16);
// pow_impl!($t, u32);
// pow_impl!($t, u64);

Maybe I should have created an issue first. I apologize if discussing the idea here is quite inappropriate.

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 this pull request may close these issues.

2 participants