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 power function (clean up) #76

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CyDragon80
Copy link
Contributor

@CyDragon80 CyDragon80 commented Dec 5, 2018

This is basically a clean up of the previous PR: #62.

Adds power function. See original PR for previous details and discussion. This also fixes a couple places where multiply could return a signed zero when the original was unsigned (which was mentioned in #72).

src/long.js Outdated
LongPrototype.power = function power(exp) {
var a = this;
if (a.eq(Long.ONE)) return a;
if (isLong(exp)) exp = exp.toInt();
Copy link
Owner

Choose a reason for hiding this comment

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

It's certainly questionable to use such a large exponent, of course, but I wonder if this is safe in that a value > 0x7fffffff respectively > 0xffffffff still yields a somewhat expected result?

Copy link
Contributor Author

@CyDragon80 CyDragon80 Dec 5, 2018

Choose a reason for hiding this comment

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

Would the preference be to keep exponent as a Long and manipulate it as a 64 bit value? I figured it would be faster to use 32 bit math, as exponent values larger than 64 would overflow on bases beyond 0 and 1 anyway. So at the time the thought was speed rather than safety. I could certainly put the 64 bit exp math back into it if that is desired. Just let me know.

Copy link
Contributor Author

@CyDragon80 CyDragon80 Dec 5, 2018

Choose a reason for hiding this comment

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

Or is the concern that it could quietly give bad values in the overflow case? Does it need some kind of range check or overflow error of some kind? One could throw if base > 1 and exp > 64 but that would only be a broad check and there are plenty of combos that would still overflow. Not sure if there is a way to detect overflows during the multiplication?
EDIT:
Though if there are only up to 64 possible integer exponents, one could have a look up table of the 64 max possible bases and compare those to the given base to potentially predict an overflow? (Well, 63 entries as one is special case... Actually 62, 2^64 would be overflow as max is one less than count.)

@CyDragon80
Copy link
Contributor Author

It is probably best not to let that operation silently overflow without throwing some kind of meaningful error. I think I've figured out how to check for overflow in a relatively efficient manner. If there is a cleaner way to go about it, I am open to suggestions.

@CyDragon80
Copy link
Contributor Author

Looking back at this, I realized I could catch floating point exponents easily enough in the overflow check, so I tweaked it and added the necessary test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants