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

added math.pow support for integer types. resolves #1637 #1642

Merged
merged 2 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@emekoi
Copy link
Contributor

emekoi commented Oct 6, 2018

  • added math.powi
  • changed math.pow to math.powf

@emekoi emekoi changed the title added math.pow support for integer types. solves #1637 added math.pow support for integer types. resolves#1637 Oct 6, 2018

@emekoi emekoi changed the title added math.pow support for integer types. resolves#1637 added math.pow support for integer types. resolves #1637 Oct 6, 2018

@emekoi emekoi force-pushed the emekoi:int-pow branch 2 times, most recently from 01de720 to ea156f1 Oct 6, 2018

@andrewrk
Copy link
Member

andrewrk left a comment

Thanks for the implementation. A couple fixups and a couple optional changes and it's good to go.

// powi(x, +-0) = 1 for any x
// powi(1, y) = 1 for any y
// powi(x, 1) = x for any x
// powi(nan, y) = nan

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 8, 2018

Member

is there a nan special case for integers? Looks like maybe all these special case comments are from floats and should be deleted.

const assert = std.debug.assert;
const assertError = std.debug.assertError;

inline fn checkFlow(comptime T: type, comptime info: builtin.TypeInfo, x: T, y: T) (error{Overflow, Underflow}!void) {

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 8, 2018

Member

It doesn't look like the semantics of this code require this function to be inline. If this function should be inline because it improves a benchmark then there should be a comment here explaining how to run the benchmark and the results with and without inlining. Otherwise functions should not be marked inline.

pub fn safePowi(comptime T: type, x: T, y: T) !T {
const info = @typeInfo(T);

switch (info) {

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 8, 2018

Member

I'd be OK with a simple comptime assert(@typeInfo(T) == builtin.TypeId.Int) rather than a switch. That would save you some indentation, and you could manually inline the checkFlow function. Your call.

if (x > 0) {
return error.Overflow;
} else {
return error.Underflow;

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 8, 2018

Member

I'm OK with this function distinguishing between Overflow and Underflow. I'm also OK with not distinguishing. It's unlikely any code calling powi will care whether it was overflow or underflow. Feel free to simplify this logic, or not. Your call.

This comment has been minimized.

Copy link
@emekoi

emekoi Oct 10, 2018

Author Contributor

i don't really think that code calling powi would care either, but i feel it's better to make the distinction between the two. it could also make debugging a bit easier.

}
}

pub fn powi(comptime T: type, x: T, y: T) T {

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 8, 2018

Member

What do you think about having std.math.pow support floats and integers, and then std.math.powi is the safe one that has a potential error?

I'd be ok with either way. On one hand having std.math.pow support integers seems clean; on the other hand, it might be nice to have different functions to point out the fact that one of them is safe (floats) and one is potentially unsafe (ints). Your call.

@emekoi emekoi force-pushed the emekoi:int-pow branch from ea156f1 to bf9b975 Oct 10, 2018

@andrewrk andrewrk merged commit a22d9da into ziglang:master Oct 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Oct 10, 2018

Great work!

@emekoi

This comment has been minimized.

Copy link
Contributor Author

emekoi commented Oct 10, 2018

thanks!

@emekoi emekoi deleted the emekoi:int-pow branch Oct 10, 2018

@numsim1415

This comment has been minimized.

Copy link

numsim1415 commented Oct 11, 2018

@emekoi Should the comments on powi.zig line 4 and 29 be that pow(0,y)=0 for any y?

@emekoi emekoi restored the emekoi:int-pow branch Oct 27, 2018

@emekoi emekoi deleted the emekoi:int-pow branch Oct 27, 2018

@emekoi

This comment has been minimized.

Copy link
Contributor Author

emekoi commented Oct 27, 2018

@numsim1415 you're right. i also messed up on line 32 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.