Skip to content

Fix Math.hypot polyfill to call with zero/one params #2117

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

Closed
wants to merge 2 commits into from

Conversation

teppeis
Copy link
Contributor

@teppeis teppeis commented Oct 30, 2016

@MatrixFrog
Copy link
Contributor

lgtm

@MatrixFrog
Copy link
Contributor

Internal review was more involved than I expected. Going to back-burner this for now.

@concavelenz
Copy link
Contributor

As I mentioned in the internal review, I don't think we want to change the type signature but it should be compatible with the actual implementation and I suggest we use V8's. Which looks like:

function MathHypot(x, y) { // Function length is 2.
    var length = arguments.length;
    var args = new Array(length);
    var max = 0;
    for (var i = 0; i < length; i++) {
      var n = arguments[i];
      if (typeof n !== "number") n = Number(n);
      if (n === Infinity || n === -Infinity) return Infinity;
      n = Math.abs(n);
      if (n > max) max = n;
      args[i] = n;
    }

    // Kahan summation to avoid rounding errors.
    // Normalize the numbers to the largest one to avoid overflow.
    if (max === 0) max = 1;
    var sum = 0;
    var compensation = 0;
    for (var i = 0; i < length; i++) {
      var n = args[i] / max;
      var summand = n * n - compensation;
      var preliminary = sum + summand;
      compensation = (preliminary - sum) - summand;
      sum = preliminary;
    }
    return Math.sqrt(sum) * max;
  };

@concavelenz
Copy link
Contributor

I changed my mind, this function isn't used enough for it to valuable to have a tighter signature.

@brad4d
Copy link
Contributor

brad4d commented Feb 2, 2017

@MatrixFrog are you proceeding with this?
@concavelenz is there any reason not to proceed. I think you cancelled out your previous objection.

@teppeis
Copy link
Contributor Author

teppeis commented Feb 3, 2017

I do not know if I should fix this PR ...
I will fix If you need.

@MatrixFrog
Copy link
Contributor

Probably not.

@MatrixFrog MatrixFrog removed their assignment Feb 3, 2017
@teppeis
Copy link
Contributor Author

teppeis commented May 1, 2018

@MatrixFrog @brad4d @concavelenz any updates?

@brad4d
Copy link
Contributor

brad4d commented Dec 20, 2018

Created internal Google issue b/121320889

@concavelenz could you determine whether we should accept or reject this PR?

@brad4d brad4d added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Dec 20, 2018
@brad4d brad4d assigned brad4d and unassigned concavelenz Jan 9, 2019
@@ -147,7 +147,6 @@ Math.sign = function(value) {};
Math.cbrt = function(value) {};

/**
* @param {number} value1
* @param {...number} var_args
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to delete the value1 argument from the parameter list, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brad4d Nice catch! I've fixed.

@brad4d
Copy link
Contributor

brad4d commented Jan 9, 2019

@concavelenz has confirmed to me that he is OK with accepting this PR.
There's one mistake I see in it for which I've added a comment.

@brad4d
Copy link
Contributor

brad4d commented Jan 16, 2019

I've imported this PR and sent it for internal testing and review.

@brad4d
Copy link
Contributor

brad4d commented Jan 28, 2019

I submitted the internal version of this change a few minutes ago.
Our next daily push to GitHub should include it and close this PR.

@tjgq tjgq closed this in 4c8936e Jan 29, 2019
@teppeis teppeis deleted the math-hypot branch January 29, 2019 01:21
@teppeis
Copy link
Contributor Author

teppeis commented Jan 29, 2019

@brad4d thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants