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

libcore: add num::Int::pow() and deprecate num::pow(). #19031

Merged
merged 1 commit into from
Nov 18, 2014

Conversation

nodakai
Copy link
Contributor

@nodakai nodakai commented Nov 17, 2014

[breaking-change]

Deprecates core::num::pow in favor of Int::pow.

/// assert_eq!(num::pow(2i, 4), 16);
/// ```
#[inline]
pub fn pow<T: Int>(mut base: T, mut exp: uint) -> T {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be #[deprecated]?

@alexcrichton
Copy link
Member

I think that the design of std::num is deliberately quite conservative in an attempt to cut down on the number of extraneous traits to ensure we can extend the design into the future. That would currently mean that the sq method would actually be part of the Int trait instead of having a new separate trait (and perhaps also defined on Float) if we were to add it. We don't currently use the traits for many convenience methods, however, so we may not want to add it at this time.

The pow method seems fine though to add to the Int trait! I would second @gankro's suggestion for leaving the old #[deprecated] function instead for now.

cc @aturon

@aturon
Copy link
Member

aturon commented Nov 17, 2014

I agree with @alexcrichton; please see the numerics reform RFC for details.

I had been thinking we could/should move pow to be a method on Int -- can you cut down the PR to just that aspect?

@aturon
Copy link
Member

aturon commented Nov 17, 2014

cc @bjz

@nodakai
Copy link
Contributor Author

nodakai commented Nov 18, 2014

@gankro Yeah, I shouldn't have been so brutal! I deprecated the free function core::num::pow() and removed the doctest (what's the official name for it?) from it because make check yields a deny(deprecated) error there otherwise.

@alexcrichton @aturon Now this PR is only about the Int::pow() method.

@brendanzab
Copy link
Member

The Int::pow method should be fine :)

/// assert_eq!(2i.pow(4), 16);
/// ```
#[inline]
fn pow(self, mut exp: uint) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Why did the implementation change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified the code by removing the special case for exp==1 because we don't have the move semantics with primitive integers. exp /= 2 is more readable than exp = exp >> 1 and equivalent to it:

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing LLVM would optimise to a bit shift anyway.

What do you mean by 'don't have the move semantics with primitive integers'?

Copy link
Member

Choose a reason for hiding this comment

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

cc. @huonw

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bjz The original code could move the base parameter to the caller as its return value when exp is 1. It could perhaps achieve better performance only with bigint objects whose initialization cost was not negligible.

@brendanzab brendanzab changed the title libcore: add Int::pow() and a new trait Sq on top of Mul. libcore: add num::Int::pow() and deprecate num::pow(). Nov 18, 2014
@brendanzab
Copy link
Member

Updated the issue title and description.

bors added a commit that referenced this pull request Nov 18, 2014
[breaking-change]

Deprecates `core::num::pow` in favor of `Int::pow`.
@bors bors closed this Nov 18, 2014
@bors bors merged commit 3fcf284 into rust-lang:master Nov 18, 2014
@nodakai nodakai deleted the libcore-pow-and-sq branch November 19, 2014 03:11
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.

7 participants